On 24 August 2011 09:18, Shashidhar Hiremath
<shashidharh@xxxxxxxxxxxxxxx> wrote:
> Hi James,
> If I understand your concern correctly, for level triggered
> interrupts,the interrupt action needs to taken first,than the
> interrupt source needs to be cleared,only then I can clear Raw
> Interrupt Status register right ?
>
> So ,accordingly ,will the below code be in appropriate order ?
> if (pending & SDMMC_INT_SDIO(i)) {
> mmc_signal_sdio_irq(slot->mmc);
> mci_writel(host, RINTSTS, SDMMC_INT_SDIO(i));
Having looked in more detail, mmc_signal_sdio_irq() disables the
interrupt and wakes up the sdio irq thread, so the order doesn't make
a difference. Clearing RINTSTS will always just cause it to get set
again until the sdio irq thread runs and clears the interrupt
condition. It still needs to be cleared somewhere though, so assuming
v2 of the patch works,
Acked-by: James Hogan <james.hogan@xxxxxxxxxx>
Cheers
James
>
>
> On Tue, Aug 23, 2011 at 7:56 PM, James Hogan <james@xxxxxxxxxxxxx> wrote:
>> Hi,
>>
>> On 23 August 2011 14:20, Shashidhar Hiremath
>> <shashidharh@xxxxxxxxxxxxxxx> wrote:
>>> The Patch adds the support for SDIO interrupts for all slots.
>>> It includes enabling of SDIO interrupts through dw_mci_enable_sdio_irq
>>> and the handling of the slot specific interrupts in the Interrupt Service
>>> Routine.
>>>
>>> Signed-off-by: Shashidhar Hiremath <shashidharh@xxxxxxxxxxxxxxx>
>>> ---
>>> v2:
>>> * As per Suggestions by James Hogan
>>> -fixed code that was disabling other interrupts.
>>> -removed [int_mask &= 0xFFFF0000; and int_mask &= 0xFFFF0000;] as it
>>> was unnecessary
>>> -modified (slot->id + 16) with appropriate Macro SDMMC_INT_SDIO(i)
>>> drivers/mmc/host/dw_mmc.c | 35 +++++++++++++++++++++++++++++++----
>>> drivers/mmc/host/dw_mmc.h | 2 +-
>>> 2 files changed, 32 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 66dcddb..b06f8f1 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -746,11 +746,29 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
>>> return present;
>>> }
>>>
>>> +static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
>>> +{
>>> + struct dw_mci_slot *slot = mmc_priv(mmc);
>>> + struct dw_mci *host = slot->host;
>>> + u32 int_mask;
>>> +
>>> + /* Enable/disable Slot Specific SDIO interrupt */
>>> + int_mask = mci_readl(host, INTMASK);
>>> + if (enb) {
>>> + mci_writel(host, INTMASK,
>>> + (int_mask | (1 << SDMMC_INT_SDIO(slot->id))));
>>> + } else {
>>> + mci_writel(host, INTMASK,
>>> + (int_mask & ~(1 << SDMMC_INT_SDIO(slot->id))));
>>> + }
>>> +}
>>> +
>>> static const struct mmc_host_ops dw_mci_ops = {
>>> - .request = dw_mci_request,
>>> - .set_ios = dw_mci_set_ios,
>>> - .get_ro = dw_mci_get_ro,
>>> - .get_cd = dw_mci_get_cd,
>>> + .request = dw_mci_request,
>>> + .set_ios = dw_mci_set_ios,
>>> + .get_ro = dw_mci_get_ro,
>>> + .get_cd = dw_mci_get_cd,
>>> + .enable_sdio_irq = dw_mci_enable_sdio_irq,
>>> };
>>>
>>> static void dw_mci_request_end(struct dw_mci *host, struct mmc_request *mrq)
>>> @@ -1179,6 +1197,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>> struct dw_mci *host = dev_id;
>>> u32 status, pending;
>>> unsigned int pass_count = 0;
>>> + int i;
>>>
>>> do {
>>> status = mci_readl(host, RINTSTS);
>>> @@ -1249,6 +1268,14 @@ static irqreturn_t dw_mci_interrupt(int irq,
>>> void *dev_id)
>>> tasklet_schedule(&host->card_tasklet);
>>> }
>>>
>>> + /* Handle SDIO Interrupts */
>>> + for (i = 0; i < host->num_slots; i++) {
>>> + struct dw_mci_slot *slot = host->slot[i];
>>> + if (pending & SDMMC_INT_SDIO(i)) {
>>> + mci_writel(host, RINTSTS, SDMMC_INT_SDIO(i));
>>> + mmc_signal_sdio_irq(slot->mmc);
>>
>> A potential problem that just occured to me, is that the TRM says (in
>> the note after the table of interrupt status bits):
>> "The SDIO interrupts, receive fifo data request (RXDR), and transmit
>> FIFO data request (txdr) are set by level-sensitive interrupt sources.
>> Therefore, the interrupt source should be first cleared before you can
>> clear the interrupt bit of the Raw interrupt register"
>>
>> I'm guessing the source would get cleared somewhere in the
>> mmc_signal_sdio_irq function, depending on the sdio driver, therefore
>> I think it'd be more correct to clear each interrupt after the
>> mmc_signal_sdio_irq() call to prevent it immediately retriggering.
>>
>> Cheers
>> James
>>
>>> + }
>>> + }
>>> } while (pass_count++ < 5);
>>>
>>> #ifdef CONFIG_MMC_DW_IDMAC
>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>> index 23c662a..ecf1043 100644
>>> --- a/drivers/mmc/host/dw_mmc.h
>>> +++ b/drivers/mmc/host/dw_mmc.h
>>> @@ -82,7 +82,7 @@
>>> #define SDMMC_CTYPE_4BIT BIT(0)
>>> #define SDMMC_CTYPE_1BIT 0
>>> /* Interrupt status & mask register defines */
>>> -#define SDMMC_INT_SDIO BIT(16)
>>> +#define SDMMC_INT_SDIO(n) BIT((16 + (n)))
>>> #define SDMMC_INT_EBE BIT(15)
>>> #define SDMMC_INT_ACD BIT(14)
>>> #define SDMMC_INT_SBE BIT(13)
>>> --
>>> 1.7.2.3
>>>
>>>
>>> --
>>> regards,
>>> Shashidhar Hiremath
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>>
>> --
>> James Hogan
>>
>
>
>
> --
> regards,
> Shashidhar Hiremath
>
--
James Hogan
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
