[PATCH 1/3] mmc: sdhci-pci: Add MSI support

[PATCH 1/3] mmc: sdhci-pci: Add MSI support

[Alexander Stein]

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/mmc/host/sdhci-pci.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 7165e6a..9382f27 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1379,6 +1379,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 
 	slots = chip->num_slots;	/* Quirk may have changed this */
 
+	pci_enable_msi(pdev);
+
 	for (i = 0; i < slots; i++) {
 		slot = sdhci_pci_probe_slot(pdev, chip, first_bar, i);
 		if (IS_ERR(slot)) {
@@ -1397,6 +1399,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
 	return 0;
 
 free:
+	pci_disable_msi(pdev);
+
 	pci_set_drvdata(pdev, NULL);
 	kfree(chip);
 
@@ -1419,6 +1423,8 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
 		for (i = 0; i < chip->num_slots; i++)
 			sdhci_pci_remove_slot(chip->slots[i]);
 
+		pci_disable_msi(pdev);
+
 		pci_set_drvdata(pdev, NULL);
 		kfree(chip);
 	}
-- 
1.7.3.4

[PATCH 2/3] mmc: sdhci: check interrupt flags in ISR again

[Alexander Stein]

When using MSI it is possible that a new MSI is sent while an earlier
MSI is currently handled. In this case SDHCI_INT_STATUS only contains
SDHCI_INT_RESPONSE and the ISR would not be called again. But at the end
of the ISR SDHCI_INT_DATA_END is now also pending which would be ignored.

Fix this by rereading the interrupt flags in the ISR until no interrupt
we care is pending.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/mmc/host/sdhci.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8d66706..654ab32 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2268,6 +2268,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	irqreturn_t result;
 	struct sdhci_host *host = dev_id;
 	u32 intmask;
+	u32 intmask_unhandled;
 	int cardint = 0;
 
 	spin_lock(&host->lock);
@@ -2286,6 +2287,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		goto out;
 	}
 
+again:
 	DBG("*** %s got interrupt: 0x%08x\n",
 		mmc_hostname(host->mmc), intmask);
 
@@ -2336,6 +2338,14 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
 	}
 
+	intmask_unhandled = intmask;
+
+	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
+
+	/* Do interrupt handling again if we got new flags */
+	if (intmask & ~intmask_unhandled)
+		goto again;
+
 	intmask &= ~SDHCI_INT_BUS_POWER;
 
 	if (intmask & SDHCI_INT_CARD_INT)
-- 
1.7.3.4

[PATCH 3/3] mmc: sdhci-pci: allow 8-bit bus width for Intel PCH

[Alexander Stein]

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/mmc/host/sdhci-pci.c |   27 +++++++++++++++++++++++++++
 include/linux/pci_ids.h      |    2 ++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 9382f27..0218cb2 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -172,6 +172,12 @@ static int mrst_hc_probe(struct sdhci_pci_chip *chip)
 	return 0;
 }
 
+static int pch_hc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+	return 0;
+}
+
 #ifdef CONFIG_PM_RUNTIME
 
 static irqreturn_t sdhci_pci_sd_cd(int irq, void *dev_id)
@@ -281,6 +287,11 @@ static const struct sdhci_pci_fixes sdhci_intel_mfd_emmc = {
 	.probe_slot	= mfd_emmc_probe_slot,
 };
 
+static const struct sdhci_pci_fixes sdhci_intel_pch_sdio = {
+	.quirks		= SDHCI_QUIRK_BROKEN_ADMA,
+	.probe_slot	= pch_hc_probe_slot,
+};
+
 /* O2Micro extra registers */
 #define O2_SD_LOCK_WP		0xD3
 #define O2_SD_MULTI_VCC3V	0xEE
@@ -817,6 +828,22 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor		= PCI_VENDOR_ID_INTEL,
+		.device		= PCI_DEVICE_ID_INTEL_PCH_SDIO0,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.driver_data	= (kernel_ulong_t)&sdhci_intel_pch_sdio,
+	},
+
+	{
+		.vendor		= PCI_VENDOR_ID_INTEL,
+		.device		= PCI_DEVICE_ID_INTEL_PCH_SDIO1,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.driver_data	= (kernel_ulong_t)&sdhci_intel_pch_sdio,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_O2,
 		.device		= PCI_DEVICE_ID_O2_8120,
 		.subvendor	= PCI_ANY_ID,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 31d77af..28cd019 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2800,6 +2800,8 @@
 #define PCI_DEVICE_ID_INTEL_82454NX     0x84cb
 #define PCI_DEVICE_ID_INTEL_84460GX	0x84ea
 #define PCI_DEVICE_ID_INTEL_IXP4XX	0x8500
+#define PCI_DEVICE_ID_INTEL_PCH_SDIO0	0x8809
+#define PCI_DEVICE_ID_INTEL_PCH_SDIO1	0x880a
 #define PCI_DEVICE_ID_INTEL_IXP2800	0x9004
 #define PCI_DEVICE_ID_INTEL_S21152BB	0xb152
 
-- 
1.7.3.4

Re: [PATCH 3/3] mmc: sdhci-pci: allow 8-bit bus width for Intel PCH

[Greg KH]

On Tue, Mar 13, 2012 at 06:16:42PM +0100, Alexander Stein wrote:
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2800,6 +2800,8 @@
>  #define PCI_DEVICE_ID_INTEL_82454NX     0x84cb
>  #define PCI_DEVICE_ID_INTEL_84460GX	0x84ea
>  #define PCI_DEVICE_ID_INTEL_IXP4XX	0x8500
> +#define PCI_DEVICE_ID_INTEL_PCH_SDIO0	0x8809
> +#define PCI_DEVICE_ID_INTEL_PCH_SDIO1	0x880a
>  #define PCI_DEVICE_ID_INTEL_IXP2800	0x9004
>  #define PCI_DEVICE_ID_INTEL_S21152BB	0xb152

Did you read the comment at the top of this file before adding your new
device ids?

If not, please do so, and redo your patch.

thanks,

greg k-h

Re: [PATCH 3/3] mmc: sdhci-pci: allow 8-bit bus width for Intel PCH

[Tomoya MORINAGA]

Hi Alexander

As a matter of interest,
do you mean that current MMC of eg20t on Linux doesn't support 8-bit access?

thanks,
-- 
ROHM Co., Ltd.
tomoya


On Wed, Mar 14, 2012 at 2:16 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |   27 +++++++++++++++++++++++++++
>  include/linux/pci_ids.h      |    2 ++
>  2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 9382f27..0218cb2 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -172,6 +172,12 @@ static int mrst_hc_probe(struct sdhci_pci_chip *chip)
>        return 0;
>  }
>
> +static int pch_hc_probe_slot(struct sdhci_pci_slot *slot)
> +{
> +       slot->host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> +       return 0;
> +}
> +
>  #ifdef CONFIG_PM_RUNTIME
>
>  static irqreturn_t sdhci_pci_sd_cd(int irq, void *dev_id)
> @@ -281,6 +287,11 @@ static const struct sdhci_pci_fixes sdhci_intel_mfd_emmc = {
>        .probe_slot     = mfd_emmc_probe_slot,
>  };
>
> +static const struct sdhci_pci_fixes sdhci_intel_pch_sdio = {
> +       .quirks         = SDHCI_QUIRK_BROKEN_ADMA,
> +       .probe_slot     = pch_hc_probe_slot,
> +};
> +
>  /* O2Micro extra registers */
>  #define O2_SD_LOCK_WP          0xD3
>  #define O2_SD_MULTI_VCC3V      0xEE
> @@ -817,6 +828,22 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
>        },
>
>        {
> +               .vendor         = PCI_VENDOR_ID_INTEL,
> +               .device         = PCI_DEVICE_ID_INTEL_PCH_SDIO0,
> +               .subvendor      = PCI_ANY_ID,
> +               .subdevice      = PCI_ANY_ID,
> +               .driver_data    = (kernel_ulong_t)&sdhci_intel_pch_sdio,
> +       },
> +
> +       {
> +               .vendor         = PCI_VENDOR_ID_INTEL,
> +               .device         = PCI_DEVICE_ID_INTEL_PCH_SDIO1,
> +               .subvendor      = PCI_ANY_ID,
> +               .subdevice      = PCI_ANY_ID,
> +               .driver_data    = (kernel_ulong_t)&sdhci_intel_pch_sdio,
> +       },
> +
> +       {
>                .vendor         = PCI_VENDOR_ID_O2,
>                .device         = PCI_DEVICE_ID_O2_8120,
>                .subvendor      = PCI_ANY_ID,
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 31d77af..28cd019 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2800,6 +2800,8 @@
>  #define PCI_DEVICE_ID_INTEL_82454NX     0x84cb
>  #define PCI_DEVICE_ID_INTEL_84460GX    0x84ea
>  #define PCI_DEVICE_ID_INTEL_IXP4XX     0x8500
> +#define PCI_DEVICE_ID_INTEL_PCH_SDIO0  0x8809
> +#define PCI_DEVICE_ID_INTEL_PCH_SDIO1  0x880a
>  #define PCI_DEVICE_ID_INTEL_IXP2800    0x9004
>  #define PCI_DEVICE_ID_INTEL_S21152BB   0xb152
>
> --
> 1.7.3.4

Re: [PATCH 3/3] mmc: sdhci-pci: allow 8-bit bus width for Intel PCH

[Alexander Stein]

Hello Tomoya,

Am Mittwoch, 14. März 2012, 10:17:35 schrieb Tomoya MORINAGA:
> Hi Alexander
> 
> As a matter of interest,
> do you mean that current MMC of eg20t on Linux doesn't support 8-bit access?

We have a board with an 8-bit connected eMMC and Linux only used 4-bit wide 
bus access. After some research IMO the device is only matched to the default 
PCI id class
> PCI_DEVICE_CLASS((PCI_CLASS_SYSTEM_SDHCI << 8), 0xFFFF00)

AFAICS it's the same issue with the Medfield eMMC where 8-bit bus support is 
added in 0d013bcf5c272faea1f8e7a5ef3cb2e98103d5cb
This also states the default is 4-bit bus width which I saw before.

Regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
August-Bebel-Str. 29
D-07973 Greiz

Tel: +49-3661-6279-0, Fax: +49-3661-6279-99
eMail:    Alexander.Stein@systec-electronic.com
Internet: http://www.systec-electronic.com

Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Jena, HRB 205563

[PATCH v2] mmc: sdhci-pci: allow 8-bit bus width for Intel PCH

[Alexander Stein]

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v2:
* moved PCI device ID definition to sdhci-pci.c. Thanks Greg for the hint

 drivers/mmc/host/sdhci-pci.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 9382f27..c9db8f8 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -29,6 +29,12 @@
 #include "sdhci.h"
 
 /*
+ * PCI device IDs
+ */
+#define PCI_DEVICE_ID_INTEL_PCH_SDIO0	0x8809
+#define PCI_DEVICE_ID_INTEL_PCH_SDIO1	0x880a
+
+/*
  * PCI registers
  */
 
@@ -172,6 +178,12 @@ static int mrst_hc_probe(struct sdhci_pci_chip *chip)
 	return 0;
 }
 
+static int pch_hc_probe_slot(struct sdhci_pci_slot *slot)
+{
+	slot->host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+	return 0;
+}
+
 #ifdef CONFIG_PM_RUNTIME
 
 static irqreturn_t sdhci_pci_sd_cd(int irq, void *dev_id)
@@ -281,6 +293,11 @@ static const struct sdhci_pci_fixes sdhci_intel_mfd_emmc = {
 	.probe_slot	= mfd_emmc_probe_slot,
 };
 
+static const struct sdhci_pci_fixes sdhci_intel_pch_sdio = {
+	.quirks		= SDHCI_QUIRK_BROKEN_ADMA,
+	.probe_slot	= pch_hc_probe_slot,
+};
+
 /* O2Micro extra registers */
 #define O2_SD_LOCK_WP		0xD3
 #define O2_SD_MULTI_VCC3V	0xEE
@@ -817,6 +834,22 @@ static const struct pci_device_id pci_ids[] __devinitdata = {
 	},
 
 	{
+		.vendor		= PCI_VENDOR_ID_INTEL,
+		.device		= PCI_DEVICE_ID_INTEL_PCH_SDIO0,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.driver_data	= (kernel_ulong_t)&sdhci_intel_pch_sdio,
+	},
+
+	{
+		.vendor		= PCI_VENDOR_ID_INTEL,
+		.device		= PCI_DEVICE_ID_INTEL_PCH_SDIO1,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID,
+		.driver_data	= (kernel_ulong_t)&sdhci_intel_pch_sdio,
+	},
+
+	{
 		.vendor		= PCI_VENDOR_ID_O2,
 		.device		= PCI_DEVICE_ID_O2_8120,
 		.subvendor	= PCI_ANY_ID,
-- 
1.7.3.4

Re: [PATCH 2/3] mmc: sdhci: check interrupt flags in ISR again

[Adrian Hunter]

On 13/03/12 19:16, Alexander Stein wrote:
> When using MSI it is possible that a new MSI is sent while an earlier
> MSI is currently handled. In this case SDHCI_INT_STATUS only contains
> SDHCI_INT_RESPONSE and the ISR would not be called again. But at the end
> of the ISR SDHCI_INT_DATA_END is now also pending which would be ignored.
> 
> Fix this by rereading the interrupt flags in the ISR until no interrupt
> we care is pending.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/mmc/host/sdhci.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8d66706..654ab32 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2268,6 +2268,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  	irqreturn_t result;
>  	struct sdhci_host *host = dev_id;
>  	u32 intmask;
> +	u32 intmask_unhandled;
>  	int cardint = 0;
>  
>  	spin_lock(&host->lock);
> @@ -2286,6 +2287,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  		goto out;
>  	}
>  
> +again:
>  	DBG("*** %s got interrupt: 0x%08x\n",
>  		mmc_hostname(host->mmc), intmask);
>  
> @@ -2336,6 +2338,14 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>  		sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
>  	}
>  
> +	intmask_unhandled = intmask;
> +
> +	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> +
> +	/* Do interrupt handling again if we got new flags */
> +	if (intmask & ~intmask_unhandled)
> +		goto again;
> +
>  	intmask &= ~SDHCI_INT_BUS_POWER;
>  
>  	if (intmask & SDHCI_INT_CARD_INT)

Why not just replace mmiowb() i.e.


diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8d66706..da8a101 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)

        result = IRQ_HANDLED;

-       mmiowb();
+       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
+       if (intmask)
+               goto again;
 out:
        spin_unlock(&host->lock);


But maybe it would be safer limiting the number of loops i.e.


diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8d66706..d88247d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2268,7 +2268,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
        irqreturn_t result;
        struct sdhci_host *host = dev_id;
        u32 intmask;
-       int cardint = 0;
+       int cardint = 0, max_loops = 16;

        spin_lock(&host->lock);

@@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)

        result = IRQ_HANDLED;

-       mmiowb();
+       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
+       if (intmask && --max_loops)
+               goto again;
 out:
        spin_unlock(&host->lock);

Re: [PATCH 2/3] mmc: sdhci: check interrupt flags in ISR again

[Alexander Stein]

Hello,

Am Mittwoch, 14. März 2012, 09:39:02 schrieb Adrian Hunter:
> On 13/03/12 19:16, Alexander Stein wrote:
> > When using MSI it is possible that a new MSI is sent while an earlier
> > MSI is currently handled. In this case SDHCI_INT_STATUS only contains
> > SDHCI_INT_RESPONSE and the ISR would not be called again. But at the end
> > of the ISR SDHCI_INT_DATA_END is now also pending which would be
> > ignored.
> > 
> > Fix this by rereading the interrupt flags in the ISR until no interrupt
> > we care is pending.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> [...]
> > @@ -2336,6 +2338,14 @@ static irqreturn_t sdhci_irq(int irq, void
> > *dev_id)> 
> >  		sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
> >  	
> >  	}
> > 
> > +	intmask_unhandled = intmask;
> > +
> > +	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> > +
> > +	/* Do interrupt handling again if we got new flags */
> > +	if (intmask & ~intmask_unhandled)
> > +		goto again;
> > +
> > 
> >  	intmask &= ~SDHCI_INT_BUS_POWER;
> >  	
> >  	if (intmask & SDHCI_INT_CARD_INT)
> 
> Why not just replace mmiowb() i.e.
> 
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8d66706..da8a101 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
> 
>         result = IRQ_HANDLED;
> 
> -       mmiowb();
> +       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> +       if (intmask)
> +               goto again;
>  out:
>         spin_unlock(&host->lock);
> 

Well, I chose this way to only printk the error once. With your suggestion it 
might be printed in each loop, dunno how often/fast these IRQ stats are set 
again after clearing. This would end in an endless loop if error flags are set 
again fast enough, but see below.
But in general I like this approach.

> But maybe it would be safer limiting the number of loops i.e.
> 
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 8d66706..d88247d 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2268,7 +2268,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>         irqreturn_t result;
>         struct sdhci_host *host = dev_id;
>         u32 intmask;
> -       int cardint = 0;
> +       int cardint = 0, max_loops = 16;
> 
>         spin_lock(&host->lock);
> 
> @@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
> 
>         result = IRQ_HANDLED;
> 
> -       mmiowb();
> +       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
> +       if (intmask && --max_loops)
> +               goto again;
>  out:
>         spin_unlock(&host->lock);

The actual problem I saw was a CMD6 command with an R1b response where the IRQ 
for the 'not busy' event was sent during ISR for the response. So I think 
normally this should only occur once.
Regarding error flags I masked the unhandled flags out in order to print an 
error only once, even if they might be set again in the next loop. With a 
simple check on intmask they might occur up to 16 times in the kernel log.
IMHO it makes no sense to repeatedly print errors about interrupt flags we 
don't handle.

Suggestions to get a more clean way?

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
August-Bebel-Str. 29
D-07973 Greiz

Tel: +49-3661-6279-0, Fax: +49-3661-6279-99
eMail:    Alexander.Stein@systec-electronic.com
Internet: http://www.systec-electronic.com

Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Jena, HRB 205563

Re: [PATCH 2/3] mmc: sdhci: check interrupt flags in ISR again

[Adrian Hunter]

On 14/03/12 09:53, Alexander Stein wrote:
> Hello,
> 
> Am Mittwoch, 14. März 2012, 09:39:02 schrieb Adrian Hunter:
>> On 13/03/12 19:16, Alexander Stein wrote:
>>> When using MSI it is possible that a new MSI is sent while an earlier
>>> MSI is currently handled. In this case SDHCI_INT_STATUS only contains
>>> SDHCI_INT_RESPONSE and the ISR would not be called again. But at the end
>>> of the ISR SDHCI_INT_DATA_END is now also pending which would be
>>> ignored.
>>>
>>> Fix this by rereading the interrupt flags in the ISR until no interrupt
>>> we care is pending.
>>>
>>> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
>> [...]
>>> @@ -2336,6 +2338,14 @@ static irqreturn_t sdhci_irq(int irq, void
>>> *dev_id)> 
>>>  		sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
>>>  	
>>>  	}
>>>
>>> +	intmask_unhandled = intmask;
>>> +
>>> +	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
>>> +
>>> +	/* Do interrupt handling again if we got new flags */
>>> +	if (intmask & ~intmask_unhandled)
>>> +		goto again;
>>> +
>>>
>>>  	intmask &= ~SDHCI_INT_BUS_POWER;
>>>  	
>>>  	if (intmask & SDHCI_INT_CARD_INT)
>>
>> Why not just replace mmiowb() i.e.
>>
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 8d66706..da8a101 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>
>>         result = IRQ_HANDLED;
>>
>> -       mmiowb();
>> +       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
>> +       if (intmask)
>> +               goto again;
>>  out:
>>         spin_unlock(&host->lock);
>>
> 
> Well, I chose this way to only printk the error once. With your suggestion it 
> might be printed in each loop, dunno how often/fast these IRQ stats are set 
> again after clearing. This would end in an endless loop if error flags are set 
> again fast enough, but see below.
> But in general I like this approach.
> 
>> But maybe it would be safer limiting the number of loops i.e.
>>
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 8d66706..d88247d 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2268,7 +2268,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>         irqreturn_t result;
>>         struct sdhci_host *host = dev_id;
>>         u32 intmask;
>> -       int cardint = 0;
>> +       int cardint = 0, max_loops = 16;
>>
>>         spin_lock(&host->lock);
>>
>> @@ -2353,7 +2353,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
>>
>>         result = IRQ_HANDLED;
>>
>> -       mmiowb();
>> +       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
>> +       if (intmask && --max_loops)
>> +               goto again;
>>  out:
>>         spin_unlock(&host->lock);
> 
> The actual problem I saw was a CMD6 command with an R1b response where the IRQ 
> for the 'not busy' event was sent during ISR for the response. So I think 
> normally this should only occur once.
> Regarding error flags I masked the unhandled flags out in order to print an 
> error only once, even if they might be set again in the next loop. With a 
> simple check on intmask they might occur up to 16 times in the kernel log.
> IMHO it makes no sense to repeatedly print errors about interrupt flags we 
> don't handle.
> 
> Suggestions to get a more clean way?

I don't know about clean, but there is this:


diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8d66706..e0909da 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2267,8 +2267,8 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 {
        irqreturn_t result;
        struct sdhci_host *host = dev_id;
-       u32 intmask;
-       int cardint = 0;
+       u32 intmask, unexpected = 0;
+       int cardint = 0, max_loops = 16;

        spin_lock(&host->lock);

@@ -2344,19 +2344,24 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
        intmask &= ~SDHCI_INT_CARD_INT;

        if (intmask) {
-               pr_err("%s: Unexpected interrupt 0x%08x.\n",
-                       mmc_hostname(host->mmc), intmask);
-               sdhci_dumpregs(host);
-
+               unexpected |= intmask;
                sdhci_writel(host, intmask, SDHCI_INT_STATUS);
        }

        result = IRQ_HANDLED;

-       mmiowb();
+       intmask = sdhci_readl(host, SDHCI_INT_STATUS);
+       if (intmask && --max_loops)
+               goto again;
 out:
        spin_unlock(&host->lock);

+       if (unexpected) {
+               pr_err("%s: Unexpected interrupt 0x%08x.\n",
+                       mmc_hostname(host->mmc), unexpected);
+               sdhci_dumpregs(host);
+       }
+
        /*
         * We have to delay this as it calls back into the driver.
         */

[PATCH v2] mmc: sdhci: check interrupt flags in ISR again

[Alexander Stein]

When using MSI it is possible that a new MSI is sent while an earlier
MSI is currently handled. In this case SDHCI_INT_STATUS only contains
SDHCI_INT_RESPONSE and the ISR would not be called again. But at the end
of the ISR SDHCI_INT_DATA_END is now also pending which would be ignored.

Fix this by rereading the interrupt flags in the ISR until no interrupt
we care is pending.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v2:
* rearranged code a bit and added a maximum loop counter as suggested
  by Adrian

 drivers/mmc/host/sdhci.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8d66706..4fe4461 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2267,8 +2267,8 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 {
 	irqreturn_t result;
 	struct sdhci_host *host = dev_id;
-	u32 intmask;
-	int cardint = 0;
+	u32 intmask, unexpected = 0;
+	int cardint = 0, max_loops = 16;
 
 	spin_lock(&host->lock);
 
@@ -2286,6 +2286,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		goto out;
 	}
 
+again:
 	DBG("*** %s got interrupt: 0x%08x\n",
 		mmc_hostname(host->mmc), intmask);
 
@@ -2344,19 +2345,23 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	intmask &= ~SDHCI_INT_CARD_INT;
 
 	if (intmask) {
-		pr_err("%s: Unexpected interrupt 0x%08x.\n",
-			mmc_hostname(host->mmc), intmask);
-		sdhci_dumpregs(host);
-
+		unexpected |= intmask;
 		sdhci_writel(host, intmask, SDHCI_INT_STATUS);
 	}
 
 	result = IRQ_HANDLED;
 
-	mmiowb();
+	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
+	if (intmask && --max_loops)
+		goto again;
 out:
 	spin_unlock(&host->lock);
 
+	if (unexpected) {
+		pr_err("%s: Unexpected interrupt 0x%08x.\n",
+			   mmc_hostname(host->mmc), unexpected);
+		sdhci_dumpregs(host);
+	}
 	/*
 	 * We have to delay this as it calls back into the driver.
 	 */
-- 
1.7.3.4

Re: [PATCH v2] mmc: sdhci: check interrupt flags in ISR again

[Adrian Hunter]

On 14/03/12 10:52, Alexander Stein wrote:
> When using MSI it is possible that a new MSI is sent while an earlier
> MSI is currently handled. In this case SDHCI_INT_STATUS only contains
> SDHCI_INT_RESPONSE and the ISR would not be called again. But at the end
> of the ISR SDHCI_INT_DATA_END is now also pending which would be ignored.
> 
> Fix this by rereading the interrupt flags in the ISR until no interrupt
> we care is pending.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
> Changes in v2:
> * rearranged code a bit and added a maximum loop counter as suggested
>   by Adrian
> 
>  drivers/mmc/host/sdhci.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Re: [PATCH 1/3] mmc: sdhci-pci: Add MSI support

[Chris Ball]

Hi Alexander,

On Tue, Mar 13 2012, Alexander Stein wrote:
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index 7165e6a..9382f27 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -1379,6 +1379,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  
>  	slots = chip->num_slots;	/* Quirk may have changed this */
>  
> +	pci_enable_msi(pdev);
> +
>  	for (i = 0; i < slots; i++) {
>  		slot = sdhci_pci_probe_slot(pdev, chip, first_bar, i);
>  		if (IS_ERR(slot)) {
> @@ -1397,6 +1399,8 @@ static int __devinit sdhci_pci_probe(struct pci_dev *pdev,
>  	return 0;
>  
>  free:
> +	pci_disable_msi(pdev);
> +
>  	pci_set_drvdata(pdev, NULL);
>  	kfree(chip);
>  
> @@ -1419,6 +1423,8 @@ static void __devexit sdhci_pci_remove(struct pci_dev *pdev)
>  		for (i = 0; i < chip->num_slots; i++)
>  			sdhci_pci_remove_slot(chip->slots[i]);
>  
> +		pci_disable_msi(pdev);
> +
>  		pci_set_drvdata(pdev, NULL);
>  		kfree(chip);
>  	}

Thanks, I've merged all three patches to mmc-next for 3.4, using v2 of
patches 2 and 3, and adding Adrian's ACK to patch 2.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child