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
From: Stefan Nilsson XK <[email protected]>
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 <[email protected]>
Acked-by: Ulf Hansson <[email protected]>
---
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, ®);
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
2011/5/4 Per Forlin <[email protected]>:
> From: Stefan Nilsson XK <[email protected]>
>
> 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
2011/5/4 Micha? Miros?aw <[email protected]>:
> 2011/5/4 Per Forlin <[email protected]>:
>> From: Stefan Nilsson XK <[email protected]>
>>
>> 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
On Wed, 4 May 2011, Per Forlin wrote:
> From: Stefan Nilsson XK <[email protected]>
>
> 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 <[email protected]>
> Acked-by: Ulf Hansson <[email protected]>
> ---
> 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 <[email protected]>
Nicolas
On Wed, 4 May 2011, Per Forlin wrote:
> 2011/5/4 Micha? Miros?aw <[email protected]>:
> > 2011/5/4 Per Forlin <[email protected]>:
> >> From: Stefan Nilsson XK <[email protected]>
> >>
> >> 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
On 4 May 2011 19:34, Nicolas Pitre <[email protected]> wrote:
> On Wed, 4 May 2011, Per Forlin wrote:
>
>> From: Stefan Nilsson XK <[email protected]>
>>
>> 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 <[email protected]>
>> Acked-by: Ulf Hansson <[email protected]>
>> ---
>> ?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 <[email protected]>
>
>
> Nicolas
>
On Wed, 4 May 2011, Per Forlin wrote:
> On 4 May 2011 19:34, Nicolas Pitre <[email protected]> wrote:
> > On Wed, 4 May 2011, Per Forlin wrote:
> >
> >> From: Stefan Nilsson XK <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Acked-by: Ulf Hansson <[email protected]>
> >> ---
> >> ?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