[PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Per Forlin]

Optimize performance for single irq

Changes since v1.
 * Add member to hold sdio_single_irq function in mmc_card and
   minimize new code in process_sdio_pending_irqs
 * Clarify commit message

Stefan Nilsson XK (1):
  sdio: optimized SDIO IRQ handling for single irq

 drivers/mmc/core/sdio_irq.c |   30 ++++++++++++++++++++++++++++++
 include/linux/mmc/card.h    |    1 +
 2 files changed, 31 insertions(+), 0 deletions(-)

-- 
1.7.4.1

[PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Per Forlin]

From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>

If there is only 1 function registered it is possible to
improve performance by directly calling the irq handler
and avoiding the overhead of reading the CCCR registers.

Signed-off-by: Per Forlin <per.forlin@linaro.org>
Acked-by: Ulf Hansson <ulf.hansson@stericsson.com>
---
 drivers/mmc/core/sdio_irq.c |   30 ++++++++++++++++++++++++++++++
 include/linux/mmc/card.h    |    1 +
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index b300161..64c4409 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -32,6 +32,16 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
 	int i, ret, count;
 	unsigned char pending;
 
+	/*
+	 * Optimization, if there is only 1 function registered
+	 * call irq handler directly
+	 */
+	if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
+		struct sdio_func *func = card->sdio_single_irq;
+		func->irq_handler(func);
+		return 1;
+	}
+
 	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_INTx, 0, &pending);
 	if (ret) {
 		printk(KERN_DEBUG "%s: error %d reading SDIO_CCCR_INTx\n",
@@ -186,6 +196,24 @@ static int sdio_card_irq_put(struct mmc_card *card)
 	return 0;
 }
 
+/* If there is only 1 function registered set sdio_single_irq */
+static void sdio_single_irq_set(struct mmc_card *card)
+{
+	struct sdio_func *func;
+	int i;
+
+	card->sdio_single_irq = NULL;
+	if ((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];
+		       if (func && func->irq_handler) {
+			       card->sdio_single_irq = func;
+			       break;
+		       }
+	       }
+}
+
 /**
  *	sdio_claim_irq - claim the IRQ for a SDIO function
  *	@func: SDIO function
@@ -227,6 +255,7 @@ int sdio_claim_irq(struct sdio_func *func, sdio_irq_handler_t *handler)
 	ret = sdio_card_irq_get(func->card);
 	if (ret)
 		func->irq_handler = NULL;
+	sdio_single_irq_set(func->card);
 
 	return ret;
 }
@@ -251,6 +280,7 @@ int sdio_release_irq(struct sdio_func *func)
 	if (func->irq_handler) {
 		func->irq_handler = NULL;
 		sdio_card_irq_put(func->card);
+		sdio_single_irq_set(func->card);
 	}
 
 	ret = mmc_io_rw_direct(func->card, 0, 0, SDIO_CCCR_IENx, 0, &reg);
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index adb4888..0d64211 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -145,6 +145,7 @@ struct mmc_card {
 	struct sdio_cccr	cccr;		/* common card info */
 	struct sdio_cis		cis;		/* common tuple info */
 	struct sdio_func	*sdio_func[SDIO_MAX_FUNCS]; /* SDIO functions (devices) */
+	struct sdio_func	*sdio_single_irq; /* SDIO function when only one IRQ active */
 	unsigned		num_info;	/* number of info strings */
 	const char		**info;		/* info strings */
 	struct sdio_func_tuple	*tuples;	/* unknown common tuples */
-- 
1.7.4.1

Re: [PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Michał Mirosław]

2011/5/4 Per Forlin <per.forlin@linaro.org>:
> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
>
> If there is only 1 function registered it is possible to
> improve performance by directly calling the irq handler
> and avoiding the overhead of reading the CCCR registers.
>
[...]
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -32,6 +32,16 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
>        int i, ret, count;
>        unsigned char pending;
>
> +       /*
> +        * Optimization, if there is only 1 function registered
> +        * call irq handler directly
> +        */
> +       if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
> +               struct sdio_func *func = card->sdio_single_irq;
> +               func->irq_handler(func);
> +               return 1;
> +       }
[...]

The second condition can be avoided:

in process_sdio_pending_irqs():

if (card->sdio_irq_func)
   call handler and return

in sdio_claim_irq():

  card->func->irq_handler = ...
  if (host->sdio_irqs == 1)
    card->sdio_irq_func = func;
  else
    card->sdio_irq_func = NULL;

in sdio_release_irq():

  card->sdio_irq_func = NULL;
  card->func->irq_handler = ...
  sdio_card_irq_put();
  if (host->sdio_irqs == 1)
    sdio_single_irq_set(func->card);

in struct mmc_card:
  struct sdio_func        *sdio_irq_func;

Best Regards,
Michał Mirosław

Re: [PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Per Forlin]

2011/5/4 Michał Mirosław <mirqus@gmail.com>:
> 2011/5/4 Per Forlin <per.forlin@linaro.org>:
>> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
>>
>> If there is only 1 function registered it is possible to
>> improve performance by directly calling the irq handler
>> and avoiding the overhead of reading the CCCR registers.
>>
> [...]
>> --- a/drivers/mmc/core/sdio_irq.c
>> +++ b/drivers/mmc/core/sdio_irq.c
>> @@ -32,6 +32,16 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
>>        int i, ret, count;
>>        unsigned char pending;
>>
>> +       /*
>> +        * Optimization, if there is only 1 function registered
>> +        * call irq handler directly
>> +        */
>> +       if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
>> +               struct sdio_func *func = card->sdio_single_irq;
>> +               func->irq_handler(func);
>> +               return 1;
>> +       }
> [...]
>
> The second condition can be avoided:
>
> in process_sdio_pending_irqs():
>
> if (card->sdio_irq_func)
>   call handler and return
>
I added the second condition as a sanity check. Same check is used in
the main for loop
>	ret = -EINVAL;
>			} else if (func->irq_handler) {
>				func->irq_handler(func);
Is the second check necessary here?

> in sdio_claim_irq():
>
>  card->func->irq_handler = ...
>  if (host->sdio_irqs == 1)
>    card->sdio_irq_func = func;
>  else
>    card->sdio_irq_func = NULL;
I wanted to keep it simple and use same function in claim and release.
Your code looks nice.
Is if safe to not check the condition "(card->host->caps &
MMC_CAP_SDIO_IRQ)". What happens if the SDIO is in polling mode?

>
> in sdio_release_irq():
>
>  card->sdio_irq_func = NULL;
>  card->func->irq_handler = ...
>  sdio_card_irq_put();
>  if (host->sdio_irqs == 1)
>    sdio_single_irq_set(func->card);
This works for me.

>
> in struct mmc_card:
>  struct sdio_func        *sdio_irq_func;
The name sdio_single_irq indicates it is only used for single irq.
"sdio_irq_func" is too generic I think. But the your name is shorter
and makes the indentation look nicer.
Not a big deal really.

I will wait until tomorrow to post patch v3. This will give time for
other to comment as well.

> Best Regards,
> Michał Mirosław
>
Thanks for your feedback,
Per

Re: [PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2040 bytes --]

On Wed, 4 May 2011, Per Forlin wrote:

> 2011/5/4 Michał Mirosław <mirqus@gmail.com>:
> > 2011/5/4 Per Forlin <per.forlin@linaro.org>:
> >> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
> >>
> >> If there is only 1 function registered it is possible to
> >> improve performance by directly calling the irq handler
> >> and avoiding the overhead of reading the CCCR registers.
> >>
> > [...]
> >> --- a/drivers/mmc/core/sdio_irq.c
> >> +++ b/drivers/mmc/core/sdio_irq.c
> >> @@ -32,6 +32,16 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
> >>        int i, ret, count;
> >>        unsigned char pending;
> >>
> >> +       /*
> >> +        * Optimization, if there is only 1 function registered
> >> +        * call irq handler directly
> >> +        */
> >> +       if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
> >> +               struct sdio_func *func = card->sdio_single_irq;
> >> +               func->irq_handler(func);
> >> +               return 1;
> >> +       }
> > [...]
> >
> > The second condition can be avoided:
> >
> > in process_sdio_pending_irqs():
> >
> > if (card->sdio_irq_func)
> >   call handler and return
> >
> I added the second condition as a sanity check. Same check is used in
> the main for loop
> >	ret = -EINVAL;
> >			} else if (func->irq_handler) {
> >				func->irq_handler(func);
> Is the second check necessary here?

Yes because we want to be notified if the hardware returns pending 
interrupt flags for interrupts we didn't claim.

> > in sdio_claim_irq():
> >
> >  card->func->irq_handler = ...
> >  if (host->sdio_irqs == 1)
> >    card->sdio_irq_func = func;
> >  else
> >    card->sdio_irq_func = NULL;
> I wanted to keep it simple and use same function in claim and release.
> Your code looks nice.
> Is if safe to not check the condition "(card->host->caps &
> MMC_CAP_SDIO_IRQ)". What happens if the SDIO is in polling mode?

You cannot avoid checking MMC_CAP_SDIO_IRQ.  If it isn't set the CCCr 
register must be polled in all cases.


Nicolas

Re: [PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Nicolas Pitre]

On Wed, 4 May 2011, Per Forlin wrote:

> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
> 
> If there is only 1 function registered it is possible to
> improve performance by directly calling the irq handler
> and avoiding the overhead of reading the CCCR registers.
> 
> Signed-off-by: Per Forlin <per.forlin@linaro.org>
> Acked-by: Ulf Hansson <ulf.hansson@stericsson.com>
> ---
>  drivers/mmc/core/sdio_irq.c |   30 ++++++++++++++++++++++++++++++
>  include/linux/mmc/card.h    |    1 +
>  2 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index b300161..64c4409 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -32,6 +32,16 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
>  	int i, ret, count;
>  	unsigned char pending;
>  
> +	/*
> +	 * Optimization, if there is only 1 function registered
> +	 * call irq handler directly
> +	 */
> +	if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
> +		struct sdio_func *func = card->sdio_single_irq;
> +		func->irq_handler(func);

I think there is little point using a func variable here, especially 
since you already reference the handler pointer in the if() statement.  
Hence:

	if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
		card->sdio_single_irq->irq_handler();
		return 1;
	}

> @@ -186,6 +196,24 @@ static int sdio_card_irq_put(struct mmc_card *card)
>  	return 0;
>  }
>  
> +/* If there is only 1 function registered set sdio_single_irq */
> +static void sdio_single_irq_set(struct mmc_card *card)
> +{

The comment is slightly wrong.  This should say "only 1 function 
interrupt registered..."  Nothing prevents this from working with 
multiple functions if only one of them has claimed an interrupt. 

Other than that:

Reviewed-by: Nicolas Pitre <nicolas.pitre@linaro.org>


Nicolas

Re: [PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Per Forlin]

On 4 May 2011 19:34, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Wed, 4 May 2011, Per Forlin wrote:
>
>> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
>>
>> If there is only 1 function registered it is possible to
>> improve performance by directly calling the irq handler
>> and avoiding the overhead of reading the CCCR registers.
>>
>> Signed-off-by: Per Forlin <per.forlin@linaro.org>
>> Acked-by: Ulf Hansson <ulf.hansson@stericsson.com>
>> ---
>>  drivers/mmc/core/sdio_irq.c |   30 ++++++++++++++++++++++++++++++
>>  include/linux/mmc/card.h    |    1 +
>>  2 files changed, 31 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
>> index b300161..64c4409 100644
>> --- a/drivers/mmc/core/sdio_irq.c
>> +++ b/drivers/mmc/core/sdio_irq.c
>> @@ -32,6 +32,16 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
>>       int i, ret, count;
>>       unsigned char pending;
>>
>> +     /*
>> +      * Optimization, if there is only 1 function registered
>> +      * call irq handler directly
>> +      */
>> +     if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
>> +             struct sdio_func *func = card->sdio_single_irq;
>> +             func->irq_handler(func);
>
> I think there is little point using a func variable here, especially
> since you already reference the handler pointer in the if() statement.
> Hence:
>
>        if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
>                card->sdio_single_irq->irq_handler();
>                return 1;
>        }
>
What do you think about:
+       struct sdio_func *func = card->sdio_single_irq;
+
+       /*
+        * Optimization, if there is only 1 function interrupt registered
+        * call irq handler directly
+        */
+       if (func) {
+               func->irq_handler(func);
+               return 1;
+       }

-                       struct sdio_func *func = card->sdio_func[i - 1];
+                       func = card->sdio_func[i - 1];

>> @@ -186,6 +196,24 @@ static int sdio_card_irq_put(struct mmc_card *card)
>>       return 0;
>>  }
>>
>> +/* If there is only 1 function registered set sdio_single_irq */
>> +static void sdio_single_irq_set(struct mmc_card *card)
>> +{
>
> The comment is slightly wrong.  This should say "only 1 function
> interrupt registered..."  Nothing prevents this from working with
> multiple functions if only one of them has claimed an interrupt.
>
> Other than that:
>
> Reviewed-by: Nicolas Pitre <nicolas.pitre@linaro.org>
>
>
> Nicolas
>

Re: [PATCH v2] sdio: optimized SDIO IRQ handling for single irq

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2461 bytes --]

On Wed, 4 May 2011, Per Forlin wrote:

> On 4 May 2011 19:34, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Wed, 4 May 2011, Per Forlin wrote:
> >
> >> From: Stefan Nilsson XK <stefan.xk.nilsson@stericsson.com>
> >>
> >> If there is only 1 function registered it is possible to
> >> improve performance by directly calling the irq handler
> >> and avoiding the overhead of reading the CCCR registers.
> >>
> >> Signed-off-by: Per Forlin <per.forlin@linaro.org>
> >> Acked-by: Ulf Hansson <ulf.hansson@stericsson.com>
> >> ---
> >>  drivers/mmc/core/sdio_irq.c |   30 ++++++++++++++++++++++++++++++
> >>  include/linux/mmc/card.h    |    1 +
> >>  2 files changed, 31 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> >> index b300161..64c4409 100644
> >> --- a/drivers/mmc/core/sdio_irq.c
> >> +++ b/drivers/mmc/core/sdio_irq.c
> >> @@ -32,6 +32,16 @@ static int process_sdio_pending_irqs(struct mmc_card *card)
> >>       int i, ret, count;
> >>       unsigned char pending;
> >>
> >> +     /*
> >> +      * Optimization, if there is only 1 function registered
> >> +      * call irq handler directly
> >> +      */
> >> +     if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
> >> +             struct sdio_func *func = card->sdio_single_irq;
> >> +             func->irq_handler(func);
> >
> > I think there is little point using a func variable here, especially
> > since you already reference the handler pointer in the if() statement.
> > Hence:
> >
> >        if (card->sdio_single_irq && card->sdio_single_irq->irq_handler) {
> >                card->sdio_single_irq->irq_handler();
> >                return 1;
> >        }
> >
> What do you think about:
> +       struct sdio_func *func = card->sdio_single_irq;
> +
> +       /*
> +        * Optimization, if there is only 1 function interrupt registered
> +        * call irq handler directly
> +        */
> +       if (func) {
> +               func->irq_handler(func);
> +               return 1;
> +       }

Sure, but I'd move the assignment right before the if() in that case for 
clarity:

       struct sdio_func *func;

       /*
        * Optimization, if there is only 1 function interrupt registered
        * call irq handler directly
        */
       func = card->sdio_single_irq;
       if (func) {
               func->irq_handler(func);
               return 1;
       }
       [...]


Nicolas