* [PATCH V2] mmc: sdhci: supporting PCI MSI
@ 2013-11-12 6:56 Jackey Shen
2013-11-12 7:44 ` Aaron Lu
0 siblings, 1 reply; 8+ messages in thread
From: Jackey Shen @ 2013-11-12 6:56 UTC (permalink / raw)
To: alexander.stein, manuel.lauss, hanfi, aaron.lu, linux-mmc
Cc: mcuos.com, Ray.Huang, Jackey Shen
Enable MSI support in sdhci-pci driver and provide the mechanism
to fall back to Legacy Pin-based Interrupt if MSI register fails.
Tested with SD cards on AMD platform.
Change from V1:
- implement this PCI MSI feature in sdhci-pci.c instead of sdhci.c
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-pci.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++++------
drivers/mmc/host/sdhci.h | 2 ++
include/linux/mmc/sdhci.h | 2 ++
5 files changed, 107 insertions(+), 8 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-pci.c b/drivers/mmc/host/sdhci-pci.c
index 8f75381..2ca29c0 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1143,10 +1143,68 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
slot->hw_reset(host);
}
+#ifdef CONFIG_MMC_SDHCI_PCI_MSI
+static int sdhci_pci_setup_msi(struct sdhci_host *host, irq_handler_t handler)
+{
+ 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, handler, 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_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
+{
+ return sdhci_pci_setup_msi(host, handler);
+}
+
+static void sdhci_pci_disable_msi(struct sdhci_host *host)
+{
+ struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
+
+ if (host->msi_enabled) {
+ pci_disable_msi(pdev);
+ host->msi_enabled = false;
+ }
+}
+
+#else
+
+static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
+{
+ return 0;
+}
+
+static void sdhci_pci_disable_msi(struct sdhci_host *host)
+{
+}
+#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
+
static const struct sdhci_ops sdhci_pci_ops = {
.enable_dma = sdhci_pci_enable_dma,
.platform_bus_width = sdhci_pci_bus_width,
.hw_reset = sdhci_pci_hw_reset,
+ .enable_msi = sdhci_pci_enable_msi,
+ .disable_msi = sdhci_pci_disable_msi,
};
/*****************************************************************************\
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6785fb1..4c2a1ac 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2554,6 +2554,31 @@ void sdhci_disable_irq_wakeups(struct sdhci_host *host)
}
EXPORT_SYMBOL_GPL(sdhci_disable_irq_wakeups);
+static int sdhci_request_irq(struct sdhci_host *host)
+{
+ int ret = 0;
+
+ /* try to enable MSI */
+ if (host->ops->enable_msi) {
+ ret = host->ops->enable_msi(host, sdhci_irq);
+ if (ret)
+ pr_warn("%s: Fall back to 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;
+ }
+ }
+
+ return ret;
+}
int sdhci_suspend_host(struct sdhci_host *host)
{
if (host->ops->platform_suspend)
@@ -2570,6 +2595,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);
+ if (host->ops->disable_msi)
+ host->ops->disable_msi(host);
} else {
sdhci_enable_irq_wakeups(host);
enable_irq_wake(host->irq);
@@ -2589,8 +2616,7 @@ 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);
+ ret = sdhci_request_irq(host);
if (ret)
return ret;
} else {
@@ -3212,13 +3238,9 @@ 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);
+ ret = sdhci_request_irq(host);
+ if (ret)
goto untasklet;
- }
#ifdef CONFIG_MMC_DEBUG
sdhci_dumpregs(host);
@@ -3258,6 +3280,8 @@ reset:
sdhci_reset(host, SDHCI_RESET_ALL);
sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
free_irq(host->irq, host);
+ if (host->ops->disable_msi)
+ host->ops->disable_msi(host);
#endif
untasklet:
tasklet_kill(&host->card_tasklet);
@@ -3301,6 +3325,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
free_irq(host->irq, host);
+ if (host->ops->disable_msi)
+ host->ops->disable_msi(host);
del_timer_sync(&host->timer);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0a3ed01..cbee843 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -296,6 +296,8 @@ struct sdhci_ops {
void (*adma_workaround)(struct sdhci_host *host, u32 intmask);
void (*platform_init)(struct sdhci_host *host);
void (*card_event)(struct sdhci_host *host);
+ int (*enable_msi)(struct sdhci_host *host, irq_handler_t handler);
+ void (*disable_msi)(struct sdhci_host *host);
};
#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mmc: sdhci: supporting PCI MSI
2013-11-12 6:56 [PATCH V2] mmc: sdhci: supporting PCI MSI Jackey Shen
@ 2013-11-12 7:44 ` Aaron Lu
2013-11-12 9:27 ` Jackey Shen
0 siblings, 1 reply; 8+ messages in thread
From: Aaron Lu @ 2013-11-12 7:44 UTC (permalink / raw)
To: Jackey Shen, alexander.stein, manuel.lauss, hanfi, linux-mmc
Cc: mcuos.com, Ray.Huang
On 11/12/2013 02:56 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.
>
> Tested with SD cards on AMD platform.
>
->
> Change from V1:
> - implement this PCI MSI feature in sdhci-pci.c instead of sdhci.c
Such words can be placed under the --- below so that it will not appear
in git log.
>
> Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> Reviewed-by: Huang Rui <ray.huang@amd.com>
> ---
->
Put change related info Here.
> drivers/mmc/host/Kconfig | 11 +++++++++
> drivers/mmc/host/sdhci-pci.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++++------
> drivers/mmc/host/sdhci.h | 2 ++
> include/linux/mmc/sdhci.h | 2 ++
> 5 files changed, 107 insertions(+), 8 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-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 8f75381..2ca29c0 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -1143,10 +1143,68 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
> slot->hw_reset(host);
> }
>
> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> +static int sdhci_pci_setup_msi(struct sdhci_host *host, irq_handler_t handler)
> +{
> + 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, handler, 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;
> +}
What about we only call pci_enable_msi in sdhci-pci.c and then assign
host->irq appropriately before calling sdhci_add_host, will that work?
The only difference would be, request_irq will have SHARED flag, but I
suppose that's not a problem.
> +
> +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> +{
> + return sdhci_pci_setup_msi(host, handler);
> +}
> +
> +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> +{
> + struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> +
> + if (host->msi_enabled) {
> + pci_disable_msi(pdev);
> + host->msi_enabled = false;
> + }
> +}
> +
> +#else
> +
> +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> +{
> + return 0;
> +}
> +
> +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> +{
> +}
> +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> +
> static const struct sdhci_ops sdhci_pci_ops = {
> .enable_dma = sdhci_pci_enable_dma,
> .platform_bus_width = sdhci_pci_bus_width,
> .hw_reset = sdhci_pci_hw_reset,
> + .enable_msi = sdhci_pci_enable_msi,
> + .disable_msi = sdhci_pci_disable_msi,
> };
>
> /*****************************************************************************\
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6785fb1..4c2a1ac 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2554,6 +2554,31 @@ void sdhci_disable_irq_wakeups(struct sdhci_host *host)
> }
> EXPORT_SYMBOL_GPL(sdhci_disable_irq_wakeups);
>
> +static int sdhci_request_irq(struct sdhci_host *host)
> +{
> + int ret = 0;
> +
> + /* try to enable MSI */
> + if (host->ops->enable_msi) {
> + ret = host->ops->enable_msi(host, sdhci_irq);
> + if (ret)
> + pr_warn("%s: Fall back to 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;
> + }
> + }
> +
> + return ret;
> +}
> int sdhci_suspend_host(struct sdhci_host *host)
> {
> if (host->ops->platform_suspend)
> @@ -2570,6 +2595,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);
> + if (host->ops->disable_msi)
> + host->ops->disable_msi(host);
What would happen if we do not disable/enable msi in suspend/resume host?
We have already masked all irqs with sdhci_mask_irqs anyway.
Thanks,
Aaron
> } else {
> sdhci_enable_irq_wakeups(host);
> enable_irq_wake(host->irq);
> @@ -2589,8 +2616,7 @@ 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);
> + ret = sdhci_request_irq(host);
> if (ret)
> return ret;
> } else {
> @@ -3212,13 +3238,9 @@ 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);
> + ret = sdhci_request_irq(host);
> + if (ret)
> goto untasklet;
> - }
>
> #ifdef CONFIG_MMC_DEBUG
> sdhci_dumpregs(host);
> @@ -3258,6 +3280,8 @@ reset:
> sdhci_reset(host, SDHCI_RESET_ALL);
> sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> free_irq(host->irq, host);
> + if (host->ops->disable_msi)
> + host->ops->disable_msi(host);
> #endif
> untasklet:
> tasklet_kill(&host->card_tasklet);
> @@ -3301,6 +3325,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>
> sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> free_irq(host->irq, host);
> + if (host->ops->disable_msi)
> + host->ops->disable_msi(host);
>
> del_timer_sync(&host->timer);
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0a3ed01..cbee843 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -296,6 +296,8 @@ struct sdhci_ops {
> void (*adma_workaround)(struct sdhci_host *host, u32 intmask);
> void (*platform_init)(struct sdhci_host *host);
> void (*card_event)(struct sdhci_host *host);
> + int (*enable_msi)(struct sdhci_host *host, irq_handler_t handler);
> + void (*disable_msi)(struct sdhci_host *host);
> };
>
> #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> 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 */
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mmc: sdhci: supporting PCI MSI
2013-11-12 7:44 ` Aaron Lu
@ 2013-11-12 9:27 ` Jackey Shen
2013-11-12 10:35 ` Jackey Shen
2013-11-13 0:54 ` Aaron Lu
0 siblings, 2 replies; 8+ messages in thread
From: Jackey Shen @ 2013-11-12 9:27 UTC (permalink / raw)
To: Aaron Lu
Cc: alexander.stein, manuel.lauss, hanfi, linux-mmc, mcuos.com, Ray.Huang
On Tue, Nov 12, 2013 at 03:44:40PM +0800, Aaron Lu wrote:
> On 11/12/2013 02:56 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.
> >
> > Tested with SD cards on AMD platform.
> >
>
> ->
>
> > Change from V1:
> > - implement this PCI MSI feature in sdhci-pci.c instead of sdhci.c
>
> Such words can be placed under the --- below so that it will not appear
> in git log.
>
> >
> > Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> > Reviewed-by: Huang Rui <ray.huang@amd.com>
> > ---
>
> ->
> Put change related info Here.
OK. I will update my patch.
>
> > drivers/mmc/host/Kconfig | 11 +++++++++
> > drivers/mmc/host/sdhci-pci.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
> > drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++++------
> > drivers/mmc/host/sdhci.h | 2 ++
> > include/linux/mmc/sdhci.h | 2 ++
> > 5 files changed, 107 insertions(+), 8 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-pci.c b/drivers/mmc/host/sdhci-pci.c
> > index 8f75381..2ca29c0 100644
> > --- a/drivers/mmc/host/sdhci-pci.c
> > +++ b/drivers/mmc/host/sdhci-pci.c
> > @@ -1143,10 +1143,68 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
> > slot->hw_reset(host);
> > }
> >
> > +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> > +static int sdhci_pci_setup_msi(struct sdhci_host *host, irq_handler_t handler)
> > +{
> > + 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, handler, 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;
> > +}
>
> What about we only call pci_enable_msi in sdhci-pci.c and then assign
> host->irq appropriately before calling sdhci_add_host, will that work?
> The only difference would be, request_irq will have SHARED flag, but I
> suppose that's not a problem.
>
There are 2 points for this part:
1. fall back to legacy interrupt right after MSI request fails;
2. prompt user more information about where error/warning occur.
> > +
> > +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> > +{
> > + return sdhci_pci_setup_msi(host, handler);
> > +}
> > +
> > +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> > +{
> > + struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> > +
> > + if (host->msi_enabled) {
> > + pci_disable_msi(pdev);
> > + host->msi_enabled = false;
> > + }
> > +}
> > +
> > +#else
> > +
> > +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> > +{
> > + return 0;
> > +}
> > +
> > +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> > +{
> > +}
> > +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> > +
> > static const struct sdhci_ops sdhci_pci_ops = {
> > .enable_dma = sdhci_pci_enable_dma,
> > .platform_bus_width = sdhci_pci_bus_width,
> > .hw_reset = sdhci_pci_hw_reset,
> > + .enable_msi = sdhci_pci_enable_msi,
> > + .disable_msi = sdhci_pci_disable_msi,
> > };
> >
> > /*****************************************************************************\
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 6785fb1..4c2a1ac 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -2554,6 +2554,31 @@ void sdhci_disable_irq_wakeups(struct sdhci_host *host)
> > }
> > EXPORT_SYMBOL_GPL(sdhci_disable_irq_wakeups);
> >
> > +static int sdhci_request_irq(struct sdhci_host *host)
> > +{
> > + int ret = 0;
> > +
> > + /* try to enable MSI */
> > + if (host->ops->enable_msi) {
> > + ret = host->ops->enable_msi(host, sdhci_irq);
> > + if (ret)
> > + pr_warn("%s: Fall back to 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;
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > int sdhci_suspend_host(struct sdhci_host *host)
> > {
> > if (host->ops->platform_suspend)
> > @@ -2570,6 +2595,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);
> > + if (host->ops->disable_msi)
> > + host->ops->disable_msi(host);
>
> What would happen if we do not disable/enable msi in suspend/resume host?
> We have already masked all irqs with sdhci_mask_irqs anyway.
>
We should also remove request|free_irq at the same time, right?
I will do some tests.
Thanks,
Jackey
> Thanks,
> Aaron
>
> > } else {
> > sdhci_enable_irq_wakeups(host);
> > enable_irq_wake(host->irq);
> > @@ -2589,8 +2616,7 @@ 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);
> > + ret = sdhci_request_irq(host);
> > if (ret)
> > return ret;
> > } else {
> > @@ -3212,13 +3238,9 @@ 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);
> > + ret = sdhci_request_irq(host);
> > + if (ret)
> > goto untasklet;
> > - }
> >
> > #ifdef CONFIG_MMC_DEBUG
> > sdhci_dumpregs(host);
> > @@ -3258,6 +3280,8 @@ reset:
> > sdhci_reset(host, SDHCI_RESET_ALL);
> > sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > free_irq(host->irq, host);
> > + if (host->ops->disable_msi)
> > + host->ops->disable_msi(host);
> > #endif
> > untasklet:
> > tasklet_kill(&host->card_tasklet);
> > @@ -3301,6 +3325,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> >
> > sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > free_irq(host->irq, host);
> > + if (host->ops->disable_msi)
> > + host->ops->disable_msi(host);
> >
> > del_timer_sync(&host->timer);
> >
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index 0a3ed01..cbee843 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -296,6 +296,8 @@ struct sdhci_ops {
> > void (*adma_workaround)(struct sdhci_host *host, u32 intmask);
> > void (*platform_init)(struct sdhci_host *host);
> > void (*card_event)(struct sdhci_host *host);
> > + int (*enable_msi)(struct sdhci_host *host, irq_handler_t handler);
> > + void (*disable_msi)(struct sdhci_host *host);
> > };
> >
> > #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> > 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mmc: sdhci: supporting PCI MSI
2013-11-12 9:27 ` Jackey Shen
@ 2013-11-12 10:35 ` Jackey Shen
2013-11-13 0:54 ` Aaron Lu
1 sibling, 0 replies; 8+ messages in thread
From: Jackey Shen @ 2013-11-12 10:35 UTC (permalink / raw)
To: Aaron Lu
Cc: alexander.stein, manuel.lauss, hanfi, linux-mmc, mcuos.com, Ray.Huang
On Tue, Nov 12, 2013 at 05:27:16PM +0800, Jackey Shen wrote:
> On Tue, Nov 12, 2013 at 03:44:40PM +0800, Aaron Lu wrote:
> > On 11/12/2013 02:56 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.
> > >
> > > Tested with SD cards on AMD platform.
> > >
> >
> > ->
> >
> > > Change from V1:
> > > - implement this PCI MSI feature in sdhci-pci.c instead of sdhci.c
> >
> > Such words can be placed under the --- below so that it will not appear
> > in git log.
> >
> > >
> > > Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> > > Reviewed-by: Huang Rui <ray.huang@amd.com>
> > > ---
> >
> > ->
> > Put change related info Here.
>
> OK. I will update my patch.
>
> >
> > > drivers/mmc/host/Kconfig | 11 +++++++++
> > > drivers/mmc/host/sdhci-pci.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
> > > drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++++------
> > > drivers/mmc/host/sdhci.h | 2 ++
> > > include/linux/mmc/sdhci.h | 2 ++
> > > 5 files changed, 107 insertions(+), 8 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-pci.c b/drivers/mmc/host/sdhci-pci.c
> > > index 8f75381..2ca29c0 100644
> > > --- a/drivers/mmc/host/sdhci-pci.c
> > > +++ b/drivers/mmc/host/sdhci-pci.c
> > > @@ -1143,10 +1143,68 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
> > > slot->hw_reset(host);
> > > }
> > >
> > > +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> > > +static int sdhci_pci_setup_msi(struct sdhci_host *host, irq_handler_t handler)
> > > +{
> > > + 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, handler, 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;
> > > +}
> >
> > What about we only call pci_enable_msi in sdhci-pci.c and then assign
> > host->irq appropriately before calling sdhci_add_host, will that work?
> > The only difference would be, request_irq will have SHARED flag, but I
> > suppose that's not a problem.
> >
> There are 2 points for this part:
> 1. fall back to legacy interrupt right after MSI request fails;
> 2. prompt user more information about where error/warning occur.
>
> > > +
> > > +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> > > +{
> > > + return sdhci_pci_setup_msi(host, handler);
> > > +}
> > > +
> > > +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> > > +{
> > > + struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> > > +
> > > + if (host->msi_enabled) {
> > > + pci_disable_msi(pdev);
> > > + host->msi_enabled = false;
> > > + }
> > > +}
> > > +
> > > +#else
> > > +
> > > +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> > > +{
> > > +}
> > > +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> > > +
> > > static const struct sdhci_ops sdhci_pci_ops = {
> > > .enable_dma = sdhci_pci_enable_dma,
> > > .platform_bus_width = sdhci_pci_bus_width,
> > > .hw_reset = sdhci_pci_hw_reset,
> > > + .enable_msi = sdhci_pci_enable_msi,
> > > + .disable_msi = sdhci_pci_disable_msi,
> > > };
> > >
> > > /*****************************************************************************\
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index 6785fb1..4c2a1ac 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -2554,6 +2554,31 @@ void sdhci_disable_irq_wakeups(struct sdhci_host *host)
> > > }
> > > EXPORT_SYMBOL_GPL(sdhci_disable_irq_wakeups);
> > >
> > > +static int sdhci_request_irq(struct sdhci_host *host)
> > > +{
> > > + int ret = 0;
> > > +
> > > + /* try to enable MSI */
> > > + if (host->ops->enable_msi) {
> > > + ret = host->ops->enable_msi(host, sdhci_irq);
> > > + if (ret)
> > > + pr_warn("%s: Fall back to 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;
> > > + }
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > int sdhci_suspend_host(struct sdhci_host *host)
> > > {
> > > if (host->ops->platform_suspend)
> > > @@ -2570,6 +2595,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);
> > > + if (host->ops->disable_msi)
> > > + host->ops->disable_msi(host);
> >
> > What would happen if we do not disable/enable msi in suspend/resume host?
> > We have already masked all irqs with sdhci_mask_irqs anyway.
> >
> We should also remove request|free_irq at the same time, right?
> I will do some tests.
Hi Aaron,
I did some tests and find system works fine:
1. remove disable/enable_msi in suspend/resume host
2. remove free|request_irq in suspend/resume host
BTY, I find system also works fine if my patch is not used and
item 2 above is adopted.
The IRQ number is reserved in suspend state and re-used in resume state.
Thanks,
Jackey
>
> Thanks,
> Jackey
>
> > Thanks,
> > Aaron
> >
> > > } else {
> > > sdhci_enable_irq_wakeups(host);
> > > enable_irq_wake(host->irq);
> > > @@ -2589,8 +2616,7 @@ 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);
> > > + ret = sdhci_request_irq(host);
> > > if (ret)
> > > return ret;
> > > } else {
> > > @@ -3212,13 +3238,9 @@ 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);
> > > + ret = sdhci_request_irq(host);
> > > + if (ret)
> > > goto untasklet;
> > > - }
> > >
> > > #ifdef CONFIG_MMC_DEBUG
> > > sdhci_dumpregs(host);
> > > @@ -3258,6 +3280,8 @@ reset:
> > > sdhci_reset(host, SDHCI_RESET_ALL);
> > > sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > > free_irq(host->irq, host);
> > > + if (host->ops->disable_msi)
> > > + host->ops->disable_msi(host);
> > > #endif
> > > untasklet:
> > > tasklet_kill(&host->card_tasklet);
> > > @@ -3301,6 +3325,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> > >
> > > sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> > > free_irq(host->irq, host);
> > > + if (host->ops->disable_msi)
> > > + host->ops->disable_msi(host);
> > >
> > > del_timer_sync(&host->timer);
> > >
> > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > > index 0a3ed01..cbee843 100644
> > > --- a/drivers/mmc/host/sdhci.h
> > > +++ b/drivers/mmc/host/sdhci.h
> > > @@ -296,6 +296,8 @@ struct sdhci_ops {
> > > void (*adma_workaround)(struct sdhci_host *host, u32 intmask);
> > > void (*platform_init)(struct sdhci_host *host);
> > > void (*card_event)(struct sdhci_host *host);
> > > + int (*enable_msi)(struct sdhci_host *host, irq_handler_t handler);
> > > + void (*disable_msi)(struct sdhci_host *host);
> > > };
> > >
> > > #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> > > 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mmc: sdhci: supporting PCI MSI
2013-11-12 9:27 ` Jackey Shen
2013-11-12 10:35 ` Jackey Shen
@ 2013-11-13 0:54 ` Aaron Lu
2013-11-13 5:55 ` Jackey Shen
1 sibling, 1 reply; 8+ messages in thread
From: Aaron Lu @ 2013-11-13 0:54 UTC (permalink / raw)
To: Jackey Shen
Cc: alexander.stein, manuel.lauss, hanfi, linux-mmc, mcuos.com, Ray.Huang
On 11/12/2013 05:27 PM, Jackey Shen wrote:
> On Tue, Nov 12, 2013 at 03:44:40PM +0800, Aaron Lu wrote:
>> On 11/12/2013 02:56 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.
>>>
>>> Tested with SD cards on AMD platform.
>>>
>>
>> ->
>>
>>> Change from V1:
>>> - implement this PCI MSI feature in sdhci-pci.c instead of sdhci.c
>>
>> Such words can be placed under the --- below so that it will not appear
>> in git log.
>>
>>>
>>> Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
>>> Reviewed-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>
>> ->
>> Put change related info Here.
>
> OK. I will update my patch.
>
>>
>>> drivers/mmc/host/Kconfig | 11 +++++++++
>>> drivers/mmc/host/sdhci-pci.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
>>> drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++++------
>>> drivers/mmc/host/sdhci.h | 2 ++
>>> include/linux/mmc/sdhci.h | 2 ++
>>> 5 files changed, 107 insertions(+), 8 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-pci.c b/drivers/mmc/host/sdhci-pci.c
>>> index 8f75381..2ca29c0 100644
>>> --- a/drivers/mmc/host/sdhci-pci.c
>>> +++ b/drivers/mmc/host/sdhci-pci.c
>>> @@ -1143,10 +1143,68 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
>>> slot->hw_reset(host);
>>> }
>>>
>>> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
>>> +static int sdhci_pci_setup_msi(struct sdhci_host *host, irq_handler_t handler)
>>> +{
>>> + 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, handler, 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;
>>> +}
>>
>> What about we only call pci_enable_msi in sdhci-pci.c and then assign
>> host->irq appropriately before calling sdhci_add_host, will that work?
>> The only difference would be, request_irq will have SHARED flag, but I
>> suppose that's not a problem.
>>
> There are 2 points for this part:
> 1. fall back to legacy interrupt right after MSI request fails;
If we call pci_enable_msi somewhere in sdhci_pci_probe_slot, then if it
failed, we can also fall back I suppose?
> 2. prompt user more information about where error/warning occur.
>
>>> +
>>> +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
>>> +{
>>> + return sdhci_pci_setup_msi(host, handler);
>>> +}
>>> +
>>> +static void sdhci_pci_disable_msi(struct sdhci_host *host)
>>> +{
>>> + struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
>>> +
>>> + if (host->msi_enabled) {
>>> + pci_disable_msi(pdev);
>>> + host->msi_enabled = false;
>>> + }
>>> +}
>>> +
>>> +#else
>>> +
>>> +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static void sdhci_pci_disable_msi(struct sdhci_host *host)
>>> +{
>>> +}
>>> +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
>>> +
>>> static const struct sdhci_ops sdhci_pci_ops = {
>>> .enable_dma = sdhci_pci_enable_dma,
>>> .platform_bus_width = sdhci_pci_bus_width,
>>> .hw_reset = sdhci_pci_hw_reset,
>>> + .enable_msi = sdhci_pci_enable_msi,
>>> + .disable_msi = sdhci_pci_disable_msi,
>>> };
>>>
>>> /*****************************************************************************\
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 6785fb1..4c2a1ac 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2554,6 +2554,31 @@ void sdhci_disable_irq_wakeups(struct sdhci_host *host)
>>> }
>>> EXPORT_SYMBOL_GPL(sdhci_disable_irq_wakeups);
>>>
>>> +static int sdhci_request_irq(struct sdhci_host *host)
>>> +{
>>> + int ret = 0;
>>> +
>>> + /* try to enable MSI */
>>> + if (host->ops->enable_msi) {
>>> + ret = host->ops->enable_msi(host, sdhci_irq);
>>> + if (ret)
>>> + pr_warn("%s: Fall back to 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;
>>> + }
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> int sdhci_suspend_host(struct sdhci_host *host)
>>> {
>>> if (host->ops->platform_suspend)
>>> @@ -2570,6 +2595,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);
>>> + if (host->ops->disable_msi)
>>> + host->ops->disable_msi(host);
>>
>> What would happen if we do not disable/enable msi in suspend/resume host?
>> We have already masked all irqs with sdhci_mask_irqs anyway.
>>
> We should also remove request|free_irq at the same time, right?
> I will do some tests.
The comments above free_irq says:
* on the card it drives before calling this function. The function
* does not return until any executing interrupts for this IRQ
* have completed.
It feels like a guard for the interrupt handler: after we call free_irq,
we are sure no interrupt hander is running(and since we have masked irq,
no more interrupt will occur either), so I suggest we keep it.
Thanks,
Aaron
>>> } else {
>>> sdhci_enable_irq_wakeups(host);
>>> enable_irq_wake(host->irq);
>>> @@ -2589,8 +2616,7 @@ 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);
>>> + ret = sdhci_request_irq(host);
>>> if (ret)
>>> return ret;
>>> } else {
>>> @@ -3212,13 +3238,9 @@ 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);
>>> + ret = sdhci_request_irq(host);
>>> + if (ret)
>>> goto untasklet;
>>> - }
>>>
>>> #ifdef CONFIG_MMC_DEBUG
>>> sdhci_dumpregs(host);
>>> @@ -3258,6 +3280,8 @@ reset:
>>> sdhci_reset(host, SDHCI_RESET_ALL);
>>> sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
>>> free_irq(host->irq, host);
>>> + if (host->ops->disable_msi)
>>> + host->ops->disable_msi(host);
>>> #endif
>>> untasklet:
>>> tasklet_kill(&host->card_tasklet);
>>> @@ -3301,6 +3325,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>>
>>> sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
>>> free_irq(host->irq, host);
>>> + if (host->ops->disable_msi)
>>> + host->ops->disable_msi(host);
>>>
>>> del_timer_sync(&host->timer);
>>>
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 0a3ed01..cbee843 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -296,6 +296,8 @@ struct sdhci_ops {
>>> void (*adma_workaround)(struct sdhci_host *host, u32 intmask);
>>> void (*platform_init)(struct sdhci_host *host);
>>> void (*card_event)(struct sdhci_host *host);
>>> + int (*enable_msi)(struct sdhci_host *host, irq_handler_t handler);
>>> + void (*disable_msi)(struct sdhci_host *host);
>>> };
>>>
>>> #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>>> 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
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mmc: sdhci: supporting PCI MSI
2013-11-13 0:54 ` Aaron Lu
@ 2013-11-13 5:55 ` Jackey Shen
2013-11-13 6:00 ` Jackey Shen
0 siblings, 1 reply; 8+ messages in thread
From: Jackey Shen @ 2013-11-13 5:55 UTC (permalink / raw)
To: Aaron Lu
Cc: alexander.stein, manuel.lauss, hanfi, linux-mmc, mcuos.com, Ray.Huang
On Wed, Nov 13, 2013 at 08:54:58AM +0800, Aaron Lu wrote:
> On 11/12/2013 05:27 PM, Jackey Shen wrote:
> > On Tue, Nov 12, 2013 at 03:44:40PM +0800, Aaron Lu wrote:
> >> On 11/12/2013 02:56 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.
> >>>
> >>> Tested with SD cards on AMD platform.
> >>>
> >>
> >> ->
> >>
> >>> Change from V1:
> >>> - implement this PCI MSI feature in sdhci-pci.c instead of sdhci.c
> >>
> >> Such words can be placed under the --- below so that it will not appear
> >> in git log.
> >>
> >>>
> >>> Signed-off-by: Jackey Shen <Jackey.Shen@amd.com>
> >>> Reviewed-by: Huang Rui <ray.huang@amd.com>
> >>> ---
> >>
> >> ->
> >> Put change related info Here.
> >
> > OK. I will update my patch.
> >
> >>
> >>> drivers/mmc/host/Kconfig | 11 +++++++++
> >>> drivers/mmc/host/sdhci-pci.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
> >>> drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++++++------
> >>> drivers/mmc/host/sdhci.h | 2 ++
> >>> include/linux/mmc/sdhci.h | 2 ++
> >>> 5 files changed, 107 insertions(+), 8 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-pci.c b/drivers/mmc/host/sdhci-pci.c
> >>> index 8f75381..2ca29c0 100644
> >>> --- a/drivers/mmc/host/sdhci-pci.c
> >>> +++ b/drivers/mmc/host/sdhci-pci.c
> >>> @@ -1143,10 +1143,68 @@ static void sdhci_pci_hw_reset(struct sdhci_host *host)
> >>> slot->hw_reset(host);
> >>> }
> >>>
> >>> +#ifdef CONFIG_MMC_SDHCI_PCI_MSI
> >>> +static int sdhci_pci_setup_msi(struct sdhci_host *host, irq_handler_t handler)
> >>> +{
> >>> + 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, handler, 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;
> >>> +}
> >>
> >> What about we only call pci_enable_msi in sdhci-pci.c and then assign
> >> host->irq appropriately before calling sdhci_add_host, will that work?
> >> The only difference would be, request_irq will have SHARED flag, but I
> >> suppose that's not a problem.
> >>
> > There are 2 points for this part:
> > 1. fall back to legacy interrupt right after MSI request fails;
>
> If we call pci_enable_msi somewhere in sdhci_pci_probe_slot, then if it
> failed, we can also fall back I suppose?
>
Yes, it is.
Also, sdhci_pci_disable_msi should be kept since pci_disable_msi is
pci bus releted and better not used in sdhci.c.
So, enable msi and disable msi are symmetric in style.
What's your opinion?
> > 2. prompt user more information about where error/warning occur.
> >
> >>> +
> >>> +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> >>> +{
> >>> + return sdhci_pci_setup_msi(host, handler);
> >>> +}
> >>> +
> >>> +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> >>> +{
> >>> + struct pci_dev *pdev = to_pci_dev(host->mmc->parent);
> >>> +
> >>> + if (host->msi_enabled) {
> >>> + pci_disable_msi(pdev);
> >>> + host->msi_enabled = false;
> >>> + }
> >>> +}
> >>> +
> >>> +#else
> >>> +
> >>> +static int sdhci_pci_enable_msi(struct sdhci_host *host, irq_handler_t handler)
> >>> +{
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static void sdhci_pci_disable_msi(struct sdhci_host *host)
> >>> +{
> >>> +}
> >>> +#endif /* CONFIG_MMC_SDHCI_PCI_MSI */
> >>> +
> >>> static const struct sdhci_ops sdhci_pci_ops = {
> >>> .enable_dma = sdhci_pci_enable_dma,
> >>> .platform_bus_width = sdhci_pci_bus_width,
> >>> .hw_reset = sdhci_pci_hw_reset,
> >>> + .enable_msi = sdhci_pci_enable_msi,
> >>> + .disable_msi = sdhci_pci_disable_msi,
> >>> };
> >>>
> >>> /*****************************************************************************\
> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>> index 6785fb1..4c2a1ac 100644
> >>> --- a/drivers/mmc/host/sdhci.c
> >>> +++ b/drivers/mmc/host/sdhci.c
> >>> @@ -2554,6 +2554,31 @@ void sdhci_disable_irq_wakeups(struct sdhci_host *host)
> >>> }
> >>> EXPORT_SYMBOL_GPL(sdhci_disable_irq_wakeups);
> >>>
> >>> +static int sdhci_request_irq(struct sdhci_host *host)
> >>> +{
> >>> + int ret = 0;
> >>> +
> >>> + /* try to enable MSI */
> >>> + if (host->ops->enable_msi) {
> >>> + ret = host->ops->enable_msi(host, sdhci_irq);
> >>> + if (ret)
> >>> + pr_warn("%s: Fall back to 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;
> >>> + }
> >>> + }
> >>> +
> >>> + return ret;
> >>> +}
> >>> int sdhci_suspend_host(struct sdhci_host *host)
> >>> {
> >>> if (host->ops->platform_suspend)
> >>> @@ -2570,6 +2595,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);
> >>> + if (host->ops->disable_msi)
> >>> + host->ops->disable_msi(host);
> >>
> >> What would happen if we do not disable/enable msi in suspend/resume host?
> >> We have already masked all irqs with sdhci_mask_irqs anyway.
> >>
> > We should also remove request|free_irq at the same time, right?
> > I will do some tests.
>
> The comments above free_irq says:
>
> * on the card it drives before calling this function. The function
> * does not return until any executing interrupts for this IRQ
> * have completed.
>
> It feels like a guard for the interrupt handler: after we call free_irq,
> we are sure no interrupt hander is running(and since we have masked irq,
> no more interrupt will occur either), so I suggest we keep it.
>
I did some tests according to your suggestion and it works fine.
Thanks,
Jackey
> Thanks,
> Aaron
>
> >>> } else {
> >>> sdhci_enable_irq_wakeups(host);
> >>> enable_irq_wake(host->irq);
> >>> @@ -2589,8 +2616,7 @@ 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);
> >>> + ret = sdhci_request_irq(host);
> >>> if (ret)
> >>> return ret;
> >>> } else {
> >>> @@ -3212,13 +3238,9 @@ 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);
> >>> + ret = sdhci_request_irq(host);
> >>> + if (ret)
> >>> goto untasklet;
> >>> - }
> >>>
> >>> #ifdef CONFIG_MMC_DEBUG
> >>> sdhci_dumpregs(host);
> >>> @@ -3258,6 +3280,8 @@ reset:
> >>> sdhci_reset(host, SDHCI_RESET_ALL);
> >>> sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> >>> free_irq(host->irq, host);
> >>> + if (host->ops->disable_msi)
> >>> + host->ops->disable_msi(host);
> >>> #endif
> >>> untasklet:
> >>> tasklet_kill(&host->card_tasklet);
> >>> @@ -3301,6 +3325,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> >>>
> >>> sdhci_mask_irqs(host, SDHCI_INT_ALL_MASK);
> >>> free_irq(host->irq, host);
> >>> + if (host->ops->disable_msi)
> >>> + host->ops->disable_msi(host);
> >>>
> >>> del_timer_sync(&host->timer);
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> >>> index 0a3ed01..cbee843 100644
> >>> --- a/drivers/mmc/host/sdhci.h
> >>> +++ b/drivers/mmc/host/sdhci.h
> >>> @@ -296,6 +296,8 @@ struct sdhci_ops {
> >>> void (*adma_workaround)(struct sdhci_host *host, u32 intmask);
> >>> void (*platform_init)(struct sdhci_host *host);
> >>> void (*card_event)(struct sdhci_host *host);
> >>> + int (*enable_msi)(struct sdhci_host *host, irq_handler_t handler);
> >>> + void (*disable_msi)(struct sdhci_host *host);
> >>> };
> >>>
> >>> #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
> >>> 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
> >>
> >
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mmc: sdhci: supporting PCI MSI
2013-11-13 5:55 ` Jackey Shen
@ 2013-11-13 6:00 ` Jackey Shen
2013-11-13 13:40 ` Aaron Lu
0 siblings, 1 reply; 8+ messages in thread
From: Jackey Shen @ 2013-11-13 6:00 UTC (permalink / raw)
To: Aaron Lu
Cc: alexander.stein, manuel.lauss, hanfi, linux-mmc, mcuos.com, Ray.Huang
On Wed, Nov 13, 2013 at 01:55:08PM +0800, Jackey Shen wrote:
> > >> What about we only call pci_enable_msi in sdhci-pci.c and then assign
> > >> host->irq appropriately before calling sdhci_add_host, will that work?
> > >> The only difference would be, request_irq will have SHARED flag, but I
> > >> suppose that's not a problem.
> > >>
> > > There are 2 points for this part:
> > > 1. fall back to legacy interrupt right after MSI request fails;
> >
> > If we call pci_enable_msi somewhere in sdhci_pci_probe_slot, then if it
> > failed, we can also fall back I suppose?
> >
> Yes, it is.
> But, sdhci_pci_disable_msi should be kept since pci_disable_msi is
> pci bus releted and better not used in sdhci.c.
> So, enable msi and disable msi are NOT symmetric in style.
> What's your opinion?
Sorry for missing word.
So, enable msi and disable msi are NOT symmetric in style.
Thanks,
Jackey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mmc: sdhci: supporting PCI MSI
2013-11-13 6:00 ` Jackey Shen
@ 2013-11-13 13:40 ` Aaron Lu
0 siblings, 0 replies; 8+ messages in thread
From: Aaron Lu @ 2013-11-13 13:40 UTC (permalink / raw)
To: Jackey Shen
Cc: alexander.stein, manuel.lauss, hanfi, linux-mmc, mcuos.com, Ray.Huang
On 11/13/2013 02:00 PM, Jackey Shen wrote:
> On Wed, Nov 13, 2013 at 01:55:08PM +0800, Jackey Shen wrote:
>>>>> What about we only call pci_enable_msi in sdhci-pci.c and then assign
>>>>> host->irq appropriately before calling sdhci_add_host, will that work?
>>>>> The only difference would be, request_irq will have SHARED flag, but I
>>>>> suppose that's not a problem.
>>>>>
>>>> There are 2 points for this part:
>>>> 1. fall back to legacy interrupt right after MSI request fails;
>>>
>>> If we call pci_enable_msi somewhere in sdhci_pci_probe_slot, then if it
>>> failed, we can also fall back I suppose?
>>>
>> Yes, it is.
>> But, sdhci_pci_disable_msi should be kept since pci_disable_msi is
>> pci bus releted and better not used in sdhci.c.
>> So, enable msi and disable msi are NOT symmetric in style.
>> What's your opinion?
>
> Sorry for missing word.
> So, enable msi and disable msi are NOT symmetric in style.
>
I meant something like this:
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index d7d6bc8..96461a3 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1339,12 +1339,17 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
host->quirks = chip->quirks;
host->quirks2 = chip->quirks2;
+ ret = pci_enable_msi(pdev);
+ if (ret) {
+ dev_err(&pdev->dev, "cannot enable msi\n");
+ goto cleanup;
+ }
host->irq = pdev->irq;
ret = pci_request_region(pdev, bar, mmc_hostname(host->mmc));
if (ret) {
dev_err(&pdev->dev, "cannot request region\n");
- goto cleanup;
+ goto disable_msi;
}
host->ioaddr = pci_ioremap_bar(pdev, bar);
@@ -1396,6 +1401,9 @@ unmap:
release:
pci_release_region(pdev, bar);
+disable_msi:
+ pci_disable_msi(pdev);
+
cleanup:
if (slot->data && slot->data->cleanup)
slot->data->cleanup(slot->data);
@@ -1431,6 +1439,8 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)
pci_release_region(slot->chip->pdev, slot->pci_bar);
+ pci_disable_msi(slot->chip->pdev);
+
sdhci_free_host(slot->host);
}
We can do all these MSI stuffs in sdhci-pci.c, but I'm not sure if this
is correct.
Thanks,
Aaron
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-13 13:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-12 6:56 [PATCH V2] mmc: sdhci: supporting PCI MSI Jackey Shen
2013-11-12 7:44 ` Aaron Lu
2013-11-12 9:27 ` Jackey Shen
2013-11-12 10:35 ` Jackey Shen
2013-11-13 0:54 ` Aaron Lu
2013-11-13 5:55 ` Jackey Shen
2013-11-13 6:00 ` Jackey Shen
2013-11-13 13:40 ` Aaron Lu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.