From: Per Forlin per.forlin@linaro.org
Daniel Drake reported an issue in the libertas sdio client that was triggered by the sdio_single_irq functionality. His SDIO device seems to raise an interrupt even though there are no bits set in the CCCR_INTx register. This behaviour is not supported by the sdio_single_irq feature nor the SDIO spec. The purpose of the sdio_single_irq feature is to avoid the overhead of checking the CCCR_INTx registers, this result in no error handling of the case if there is a pending IRQ with none CCCR_INTx bits set.
This patchset adds a quirk to support this spurious IRQ issue and also report a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
Changes since v1: * Replace public sdio function to enable disable SDIO single IRQ function with a quirk
Per Forlin (2): sdio: add quirk to handle pending IRQ in case of none CCCR_INTx bits sdio: report error if pending IRQ but none CCCR_INTx bits
drivers/mmc/core/sdio_irq.c | 9 +++++++-- include/linux/mmc/card.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-)
From: Per Forlin per.forlin@linaro.org
Add quirk MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 to handle SDIO device that may have pending IRQ when none bits are set in CCCR_INTx.
This IRQ issue was identified in the libertas sdio driver where the SDIO device seems to raise an interrupt even if there were none function bits in CCCR_INTx set. SDIO single irq feature will call irq handler directly in case of only one interrupt registered. It will not check the CCCR_INTx register and miss if the register is 0 and call the irq handler anyway. This IRQ issue is not defined by the SDIO spec. Use this quirk to work around this SDIO IRQ issue for a specific device.
Signed-off-by: Per Forlin per.forlin@linaro.org --- drivers/mmc/core/sdio_irq.c | 4 ++-- include/linux/mmc/card.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c index 03ead02..df1e946 100644 --- a/drivers/mmc/core/sdio_irq.c +++ b/drivers/mmc/core/sdio_irq.c @@ -204,7 +204,8 @@ static void sdio_single_irq_set(struct mmc_card *card) int i;
card->sdio_single_irq = NULL; - if ((card->host->caps & MMC_CAP_SDIO_IRQ) && + if (!(card->quirks & MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0) && + (card->host->caps & MMC_CAP_SDIO_IRQ) && card->host->sdio_irqs == 1) for (i = 0; i < card->sdio_funcs; i++) { func = card->sdio_func[i]; @@ -301,4 +302,3 @@ int sdio_release_irq(struct sdio_func *func) return 0; } EXPORT_SYMBOL_GPL(sdio_release_irq); - diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index 7b4fd7b..852021c 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -175,6 +175,7 @@ struct mmc_card { #define MMC_QUIRK_DISABLE_CD (1<<5) /* disconnect CD/DAT[3] resistor */ #define MMC_QUIRK_INAND_CMD38 (1<<6) /* iNAND devices have broken CMD38 */ #define MMC_QUIRK_BLK_NO_CMD23 (1<<7) /* Avoid CMD23 for regular multiblock */ +#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7) /* SDIO card has IRQ even if CCCR_INTx is 0 */
unsigned int erase_size; /* erase size in sectors */ unsigned int erase_shift; /* if erase unit is power 2 */
From: Per Forlin per.forlin@linaro.org
Return error in case of pending IRQ but none functions bits in CCCR_INTx are set.
Signed-off-by: Per Forlin per.forlin@linaro.org --- drivers/mmc/core/sdio_irq.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c index df1e946..4288d92 100644 --- a/drivers/mmc/core/sdio_irq.c +++ b/drivers/mmc/core/sdio_irq.c @@ -50,6 +50,11 @@ static int process_sdio_pending_irqs(struct mmc_card *card) return ret; }
+ if (!pending) { + printk(KERN_WARNING "%s: pending IRQ but none function bits\n", + mmc_card_id(card)); + ret = -EINVAL; + } count = 0; for (i = 1; i <= 7; i++) { if (pending & (1 << i)) {
On 6/1/2011 1:48 AM, Per Forlin wrote:
From: Per Forlin per.forlin@linaro.org
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index 7b4fd7b..852021c 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -175,6 +175,7 @@ struct mmc_card { #define MMC_QUIRK_DISABLE_CD (1<<5) /* disconnect CD/DAT[3] resistor */ #define MMC_QUIRK_INAND_CMD38 (1<<6) /* iNAND devices have broken CMD38 */ #define MMC_QUIRK_BLK_NO_CMD23 (1<<7) /* Avoid CMD23 for regular multiblock */ +#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7) /* SDIO card has IRQ even if CCCR_INTx is 0 */
Using the same value as MMC_QUIRK_BLK_NO_CMD23 looks odd...
On 1 June 2011 23:36, Troy Kisky troy.kisky@boundarydevices.com wrote:
On 6/1/2011 1:48 AM, Per Forlin wrote:
From: Per Forlin per.forlin@linaro.org
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index 7b4fd7b..852021c 100644 --- a/include/linux/mmc/card.h +++ b/include/linux/mmc/card.h @@ -175,6 +175,7 @@ struct mmc_card { #define MMC_QUIRK_DISABLE_CD (1<<5) /* disconnect CD/DAT[3] resistor */ #define MMC_QUIRK_INAND_CMD38 (1<<6) /* iNAND devices have broken CMD38 */ #define MMC_QUIRK_BLK_NO_CMD23 (1<<7) /* Avoid CMD23 for regular multiblock */ +#define MMC_QUIRK_SDIO_IRQ_CCCR_INTX_0 (1<<7) /* SDIO card has IRQ even if CCCR_INTx is 0 */
Using the same value as MMC_QUIRK_BLK_NO_CMD23 looks odd...
Thanks for your observation. Typo, it should be (1 << 8) I'll update the patch but I don't plan post a new version yet. The root cause issue in the libertas is resolved. If the fix turns out to be needed later on I resend it.
Thanks, Per
On Wed, 1 Jun 2011, Per Forlin wrote:
From: Per Forlin per.forlin@linaro.org
Daniel Drake reported an issue in the libertas sdio client that was triggered by the sdio_single_irq functionality. His SDIO device seems to raise an interrupt even though there are no bits set in the CCCR_INTx register. This behaviour is not supported by the sdio_single_irq feature nor the SDIO spec. The purpose of the sdio_single_irq feature is to avoid the overhead of checking the CCCR_INTx registers, this result in no error handling of the case if there is a pending IRQ with none CCCR_INTx bits set.
This patchset adds a quirk to support this spurious IRQ issue and also report a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
Given that the issue can be fixed locally to the libertas driver, I'd suggest not merging this series until truly unfixable issues come up, as Daniel said.
Nicolas
Hi,
On Wed, Jun 01 2011, Nicolas Pitre wrote:
This patchset adds a quirk to support this spurious IRQ issue and also report a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
Given that the issue can be fixed locally to the libertas driver, I'd suggest not merging this series until truly unfixable issues come up, as Daniel said.
Yep, agreed. Dan, please go ahead and submit the libertas patch and let us know if there are any problems getting it in for 3.0. (Ideally by the next -rc!)
Thanks very much, everyone,
- Chris.
On 1 June 2011 17:59, Nicolas Pitre nicolas.pitre@linaro.org wrote:
On Wed, 1 Jun 2011, Per Forlin wrote:
From: Per Forlin per.forlin@linaro.org
Daniel Drake reported an issue in the libertas sdio client that was triggered by the sdio_single_irq functionality. His SDIO device seems to raise an interrupt even though there are no bits set in the CCCR_INTx register. This behaviour is not supported by the sdio_single_irq feature nor the SDIO spec. The purpose of the sdio_single_irq feature is to avoid the overhead of checking the CCCR_INTx registers, this result in no error handling of the case if there is a pending IRQ with none CCCR_INTx bits set.
This patchset adds a quirk to support this spurious IRQ issue and also report a warning if an SDIO interrupt is raised but none CCCR_INTx bits are set.
Given that the issue can be fixed locally to the libertas driver, I'd suggest not merging this series until truly unfixable issues come up, as Daniel said.
I agree too.
Thanks, Per
-
Chris Ball -
Nicolas Pitre -
Per Forlin -
Per Forlin -
Troy Kisky