From: Juuso Oikarinen <[email protected]>
Currently the wl12xx interrupts are not configured to wake up the host, which
leads to reduced performance.
Add calls to enable waking up the host for the wl12xx irq.
Signed-off-by: Juuso Oikarinen <[email protected]>
---
drivers/net/wireless/wl12xx/sdio.c | 2 ++
drivers/net/wireless/wl12xx/spi.c | 2 ++
2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/sdio.c b/drivers/net/wireless/wl12xx/sdio.c
index d5e8748..fc0235a 100644
--- a/drivers/net/wireless/wl12xx/sdio.c
+++ b/drivers/net/wireless/wl12xx/sdio.c
@@ -84,12 +84,14 @@ static irqreturn_t wl1271_irq(int irq, void *cookie)
static void wl1271_sdio_disable_interrupts(struct wl1271 *wl)
{
+ disable_irq_wake(wl->irq);
disable_irq(wl->irq);
}
static void wl1271_sdio_enable_interrupts(struct wl1271 *wl)
{
enable_irq(wl->irq);
+ enable_irq_wake(wl->irq);
}
static void wl1271_sdio_reset(struct wl1271 *wl)
diff --git a/drivers/net/wireless/wl12xx/spi.c b/drivers/net/wireless/wl12xx/spi.c
index 0132dad..85ad1bb 100644
--- a/drivers/net/wireless/wl12xx/spi.c
+++ b/drivers/net/wireless/wl12xx/spi.c
@@ -80,12 +80,14 @@ static struct device *wl1271_spi_wl_to_dev(struct wl1271 *wl)
static void wl1271_spi_disable_interrupts(struct wl1271 *wl)
{
+ disable_irq_wake(wl->irq);
disable_irq(wl->irq);
}
static void wl1271_spi_enable_interrupts(struct wl1271 *wl)
{
enable_irq(wl->irq);
+ enable_irq_wake(wl->irq);
}
static void wl1271_spi_reset(struct wl1271 *wl)
--
1.7.1
On Tue, Feb 8, 2011 at 9:57 AM, Juuso Oikarinen
<[email protected]> wrote:
>> It might mean your platforms are configured to hit retention (assuming
>> omap) or even off mode in the cpuidle path as well, and in that case
>> it can explain the enable_irq_wake..
>>
>> > I'm currently trying to find out if this is really needed or not.
>>
>> Sure, thanks.
>
> They are saying this is needed for OMAP's retention mode too
yes, it is.
> ,so AFAIK this is needed, not just for suspend.
so your platforms are configured to hit retention in cpuidle. nice.
On Mon, 2011-02-07 at 14:09 +0200, ext Ohad Ben-Cohen wrote:
> On Mon, Feb 7, 2011 at 1:30 PM, <[email protected]> wrote:
> >
> > From: Juuso Oikarinen <[email protected]>
> >
> > Currently the wl12xx interrupts are not configured to wake up the host, which
> > leads to reduced performance.
>
> Can you elaborate what do you mean ?
>
> Today the chip is powered off at system suspend, so it won't trigger
> any interrupts anyway.
>
> When additional suspend modes will be introduced (e.g. WoWL) this will
> be needed of course.
These are not just for suspend, AFAIK, but apply also to power saving
states. Or do you know better?
-Juuso
> Thanks,
> Ohad.
Juuso Oikarinen <[email protected]> writes:
> On Tue, 2011-02-08 at 12:13 +0200, ext Kalle Valo wrote:
>> Juuso Oikarinen <[email protected]> writes:
>>
>> To me platform specific changes in drivers are always problematic.
>
> Yeah I tend to agree.
>
> But then for some reason there is this interface in interrupts.h to do
> this, and AFAIU the one requesting the IRQ and controlling enabling and
> disabling of it is supposed to also tune this wakeing on/off. Apparently
> it should even do this when just enabling/disabling the interrupt -
> which are things performed in the driver.
Yeah, you have a point. There has to be some reason for adding this.
Unfortunately the documentation I found was scarse and I didn't get
how it should be used. For example, wakeup from what? Only from
suspend? Or also from sleep states? But then again, isn't it obvious
that an irq from the device should wake the cpu from sleep states?
And to make this more generic: should all wifi drivers use
enable_irq_wake()?
(Just trying to understand this.)
--
Kalle Valo
On Mon, 2011-02-07 at 16:56 +0200, ext Ohad Ben-Cohen wrote:
> On Mon, Feb 7, 2011 at 2:58 PM, Juuso Oikarinen
> <[email protected]> wrote:
> > Actually this I don't see any performance impact for better or worse of
> > this patch on brief testing. I was instructed by some of our other
> > kernel-dudes that this is actually needed for the cpuidle path too, or
> > there might be some implications.
>
> It might mean your platforms are configured to hit retention (assuming
> omap) or even off mode in the cpuidle path as well, and in that case
> it can explain the enable_irq_wake..
>
> > I'm currently trying to find out if this is really needed or not.
>
> Sure, thanks.
They are saying this is needed for OMAP's retention mode too, so AFAIK
this is needed, not just for suspend.
-Juuso
On Mon, Feb 7, 2011 at 1:30 PM, <[email protected]> wrote:
>
> From: Juuso Oikarinen <[email protected]>
>
> Currently the wl12xx interrupts are not configured to wake up the host, which
> leads to reduced performance.
Can you elaborate what do you mean ?
Today the chip is powered off at system suspend, so it won't trigger
any interrupts anyway.
When additional suspend modes will be introduced (e.g. WoWL) this will
be needed of course.
Thanks,
Ohad.
Juuso Oikarinen <[email protected]> writes:
>> > I'm currently trying to find out if this is really needed or not.
>>
>> Sure, thanks.
>
> They are saying this is needed for OMAP's retention mode too, so AFAIK
> this is needed, not just for suspend.
Why make this change in the driver? Doesn't it mean that all drivers
need to be changed so that they will work in your platform? I would
have expected that this is done automatically somewhere in the core
kernel and without any changes to the driver.
To me platform specific changes in drivers are always problematic.
--
Kalle Valo
On Mon, Feb 7, 2011 at 2:29 PM, Juuso Oikarinen
<[email protected]> wrote:
> On Mon, 2011-02-07 at 14:09 +0200, ext Ohad Ben-Cohen wrote:
>> On Mon, Feb 7, 2011 at 1:30 PM, <[email protected]> wrote:
>> >
>> > From: Juuso Oikarinen <[email protected]>
>> >
>> > Currently the wl12xx interrupts are not configured to wake up the host, which
>> > leads to reduced performance.
>>
>> Can you elaborate what do you mean ?
>>
>> Today the chip is powered off at system suspend, so it won't trigger
>> any interrupts anyway.
>>
>> When additional suspend modes will be introduced (e.g. WoWL) this will
>> be needed of course.
>
> These are not just for suspend, AFAIK, but apply also to power saving
> states.
What power states do you mean ? do you mean the cpuidle path ? I don't
think it's related.
Can you please describe what was the reduced performance that this
solved for you ? performance is a hot topic for us now :)
Thanks,
Ohad.
Or do you know better?
>
> -Juuso
>
>> Thanks,
>> Ohad.
>
>
>
On Tue, 2011-02-08 at 12:13 +0200, ext Kalle Valo wrote:
> Juuso Oikarinen <[email protected]> writes:
>
> >> > I'm currently trying to find out if this is really needed or not.
> >>
> >> Sure, thanks.
> >
> > They are saying this is needed for OMAP's retention mode too, so AFAIK
> > this is needed, not just for suspend.
>
> Why make this change in the driver? Doesn't it mean that all drivers
> need to be changed so that they will work in your platform? I would
> have expected that this is done automatically somewhere in the core
> kernel and without any changes to the driver.
>
> To me platform specific changes in drivers are always problematic.
Yeah I tend to agree.
But then for some reason there is this interface in interrupts.h to do
this, and AFAIU the one requesting the IRQ and controlling enabling and
disabling of it is supposed to also tune this wakeing on/off. Apparently
it should even do this when just enabling/disabling the interrupt -
which are things performed in the driver.
So in this case this would need to be a flag in the interrupt
registration itself, so that the interrupt enable/disable calls could
automatically control wake-up. I guess.
-Juuso
On Mon, 2011-02-07 at 14:38 +0200, ext Ohad Ben-Cohen wrote:
> On Mon, Feb 7, 2011 at 2:29 PM, Juuso Oikarinen
> <[email protected]> wrote:
> > On Mon, 2011-02-07 at 14:09 +0200, ext Ohad Ben-Cohen wrote:
> >> On Mon, Feb 7, 2011 at 1:30 PM, <[email protected]> wrote:
> >> >
> >> > From: Juuso Oikarinen <[email protected]>
> >> >
> >> > Currently the wl12xx interrupts are not configured to wake up the host, which
> >> > leads to reduced performance.
> >>
> >> Can you elaborate what do you mean ?
> >>
> >> Today the chip is powered off at system suspend, so it won't trigger
> >> any interrupts anyway.
> >>
> >> When additional suspend modes will be introduced (e.g. WoWL) this will
> >> be needed of course.
> >
> > These are not just for suspend, AFAIK, but apply also to power saving
> > states.
>
> What power states do you mean ? do you mean the cpuidle path ? I don't
> think it's related.
>
> Can you please describe what was the reduced performance that this
> solved for you ? performance is a hot topic for us now :)
Actually this I don't see any performance impact for better or worse of
this patch on brief testing. I was instructed by some of our other
kernel-dudes that this is actually needed for the cpuidle path too, or
there might be some implications.
I'm currently trying to find out if this is really needed or not.
-Juuso
On Tue, 2011-02-08 at 13:47 +0200, ext Kalle Valo wrote:
> Juuso Oikarinen <[email protected]> writes:
>
> > On Tue, 2011-02-08 at 12:13 +0200, ext Kalle Valo wrote:
> >> Juuso Oikarinen <[email protected]> writes:
> >>
> >> To me platform specific changes in drivers are always problematic.
> >
> > Yeah I tend to agree.
> >
> > But then for some reason there is this interface in interrupts.h to do
> > this, and AFAIU the one requesting the IRQ and controlling enabling and
> > disabling of it is supposed to also tune this wakeing on/off. Apparently
> > it should even do this when just enabling/disabling the interrupt -
> > which are things performed in the driver.
>
> Yeah, you have a point. There has to be some reason for adding this.
> Unfortunately the documentation I found was scarse and I didn't get
> how it should be used. For example, wakeup from what? Only from
> suspend? Or also from sleep states? But then again, isn't it obvious
> that an irq from the device should wake the cpu from sleep states?
>
> And to make this more generic: should all wifi drivers use
> enable_irq_wake()?
>
> (Just trying to understand this.)
>
Luca, please discard this patch.
Based on the text in interrupt.h this is for waking up from suspend and
the like, which is not why I'm calling it here - the wl12xx is switched
off in suspend anyway, so no IRQ's.
As you say, it should be self-evident that a WLAN driver (or most other
drivers for that matter) will want to wake the host from
cpu-power-saving state without explicitly having to say that.
Starts to sound like there is some weird hack in our kernel tree. I'll
need to pursue that further.
-Juuso
On Mon, Feb 7, 2011 at 2:58 PM, Juuso Oikarinen
<[email protected]> wrote:
> Actually this I don't see any performance impact for better or worse of
> this patch on brief testing. I was instructed by some of our other
> kernel-dudes that this is actually needed for the cpuidle path too, or
> there might be some implications.
It might mean your platforms are configured to hit retention (assuming
omap) or even off mode in the cpuidle path as well, and in that case
it can explain the enable_irq_wake..
> I'm currently trying to find out if this is really needed or not.
Sure, thanks.