| Message ID | 1523966821-21903-5-git-send-email-pillair@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d5cded16fdc02a31f9f73d899329896218c594aa |
| Delegated to: | Kalle Valo |
| Headers | show |
from my point of view powersave should be optional and not forced. consider : iw dev <devname> set power_save on/off so there is already a config option made for that purpose, Sebastian Am 17.04.2018 um 14:07 schrieb pillair@codeaurora.org: > From: Govind Singh <govinds@codeaurora.org> > > Enable sta power save in fw for the targets that > supports idle power save. The idle ps enable command > will be ignored by the firmware which does not support > this feature. > > Signed-off-by: Govind Singh <govinds@codeaurora.org> > Signed-off-by: Rakesh Pillai <pillair@codeaurora.org> > --- > drivers/net/wireless/ath/ath10k/mac.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c > index 7e02ca02b28e..1d9222af1bb2 100644 > --- a/drivers/net/wireless/ath/ath10k/mac.c > +++ b/drivers/net/wireless/ath/ath10k/mac.c > @@ -4679,6 +4679,13 @@ static int ath10k_start(struct ieee80211_hw *hw) > } > } > > + param = ar->wmi.pdev_param->idle_ps_config; > + ret = ath10k_wmi_pdev_set_param(ar, param, 1); > + if (ret && ret != -EOPNOTSUPP) { > + ath10k_warn(ar, "failed to enable idle_ps_config: %d\n", ret); > + goto err_core_stop; > + } > + > __ath10k_set_antenna(ar, ar->cfg_tx_chainmask, ar->cfg_rx_chainmask); > > /*
On 2018-04-18 12:36, Sebastian Gottschall wrote: > from my point of view powersave should be optional and not forced. > > consider : > iw dev <devname> set power_save on/off > > so there is already a config option made for that purpose, > > Sebastian > > Am 17.04.2018 um 14:07 schrieb pillair@codeaurora.org: >> From: Govind Singh <govinds@codeaurora.org> >> >> Enable sta power save in fw for the targets that >> supports idle power save. The idle ps enable command >> will be ignored by the firmware which does not support >> this feature. >> >> Signed-off-by: Govind Singh <govinds@codeaurora.org> >> Signed-off-by: Rakesh Pillai <pillair@codeaurora.org> >> --- >> drivers/net/wireless/ath/ath10k/mac.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/net/wireless/ath/ath10k/mac.c >> b/drivers/net/wireless/ath/ath10k/mac.c >> index 7e02ca02b28e..1d9222af1bb2 100644 >> --- a/drivers/net/wireless/ath/ath10k/mac.c >> +++ b/drivers/net/wireless/ath/ath10k/mac.c >> @@ -4679,6 +4679,13 @@ static int ath10k_start(struct ieee80211_hw >> *hw) >> } >> } >> + param = ar->wmi.pdev_param->idle_ps_config; >> + ret = ath10k_wmi_pdev_set_param(ar, param, 1); >> + if (ret && ret != -EOPNOTSUPP) { >> + ath10k_warn(ar, "failed to enable idle_ps_config: %d\n", ret); >> + goto err_core_stop; >> + } >> + >> __ath10k_set_antenna(ar, ar->cfg_tx_chainmask, >> ar->cfg_rx_chainmask); >> /* Thanks Sebastian for your review. Agree this should not be forced with in driver. I will check if i can set the idle ps at the end of ath10k_mac_vif_setup_ps by checking arvif->ps. I will update the patch accordingly. BR, Govind
(fixing top posting) govinds@codeaurora.org writes: > On 2018-04-18 12:36, Sebastian Gottschall wrote: >> from my point of view powersave should be optional and not forced. >> >> consider : >> iw dev <devname> set power_save on/off >> >> so there is already a config option made for that purpose, > > Thanks Sebastian for your review. Agree this should not be forced with > in driver. > > I will check if i can set the idle ps at the end of > ath10k_mac_vif_setup_ps by checking arvif->ps. > I will update the patch accordingly. So what power save is this exactly? I assumed it's some bus level power save (turning of clocks etc) but is it actually 802.11 power save?
On 4/18/2018 3:16 PM, Kalle Valo wrote: > (fixing top posting) > > govinds@codeaurora.org writes: > >> On 2018-04-18 12:36, Sebastian Gottschall wrote: >>> from my point of view powersave should be optional and not forced. >>> >>> consider : >>> iw dev <devname> set power_save on/off >>> >>> so there is already a config option made for that purpose, >> >> Thanks Sebastian for your review. Agree this should not be forced with >> in driver. >> >> I will check if i can set the idle ps at the end of >> ath10k_mac_vif_setup_ps by checking arvif->ps. >> I will update the patch accordingly. > > So what power save is this exactly? I assumed it's some bus level power > save (turning of clocks etc) but is it actually 802.11 power save? I also suspected bus level power save and I would say runtime-pm framework should be considered for that. Regards, Arend
On 2018-04-18 18:46, Kalle Valo wrote: > (fixing top posting) > > govinds@codeaurora.org writes: > >> On 2018-04-18 12:36, Sebastian Gottschall wrote: >>> from my point of view powersave should be optional and not forced. >>> >>> consider : >>> iw dev <devname> set power_save on/off >>> >>> so there is already a config option made for that purpose, >> >> Thanks Sebastian for your review. Agree this should not be forced with >> in driver. >> >> I will check if i can set the idle ps at the end of >> ath10k_mac_vif_setup_ps by checking arvif->ps. >> I will update the patch accordingly. > > So what power save is this exactly? I assumed it's some bus level power > save (turning of clocks etc) but is it actually 802.11 power save? Yes this is WIFI chip set level power-save(based on idleness) and not related to protocol power save. FW will turn off/scale down the resources(clock/rails) based on opportunity(when ever idle mode is detected). This power save is mostly done in disconnected state. I am not really sure when framework/user-space triggers power-save config(iw dev <devname> set power_save on/off). Then doing this from user-space will be unnecessarily toggling this config or may not be setting at disconnected state. May be you can suggest better here. BR, Govind
govinds@codeaurora.org writes: > On 2018-04-18 18:46, Kalle Valo wrote: >> (fixing top posting) >> >> govinds@codeaurora.org writes: >> >>> On 2018-04-18 12:36, Sebastian Gottschall wrote: >>>> from my point of view powersave should be optional and not forced. >>>> >>>> consider : >>>> iw dev <devname> set power_save on/off >>>> >>>> so there is already a config option made for that purpose, >>> >>> Thanks Sebastian for your review. Agree this should not be forced with >>> in driver. >>> >>> I will check if i can set the idle ps at the end of >>> ath10k_mac_vif_setup_ps by checking arvif->ps. >>> I will update the patch accordingly. >> >> So what power save is this exactly? I assumed it's some bus level power >> save (turning of clocks etc) but is it actually 802.11 power save? > > Yes this is WIFI chip set level power-save(based on idleness) and not > related to protocol power save. FW will turn off/scale down the > resources(clock/rails) based on opportunity(when ever idle mode is > detected). This power save is mostly done in disconnected state. I am > not really sure when framework/user-space triggers power-save > config(iw dev <devname> set power_save on/off). Then doing this from > user-space will be unnecessarily toggling this config or may not be > setting at disconnected state. I think that 'set power_save' commands affects struct ieee80211_bss_conf::ps parameter and I don't think it should be used in this case. We already have ath10k_config_ps() for 802.11 level of power saving. Arend again proposed runtime-pm with which I'm not very familiar. But why would we want to disable this? Doesn't it make sense to have this feature always enabled to save power? wcn3990 is a chip for a mobile device anyway.
Hi, My 2c here. As much as I like power save stuff, I've been bitten enough times by these wifi chips kinda implementing mostly-ok-but-not-that-particular-time power savings stuff and so have had to make it configurable chip by chip. A good example is active state power management in various runtime and idle modes on different chips, how it intersects with things like bluetooth, btcoex, is the bios buggy, etc, etc. So I'd at least like a way to control that feature as a module load and/or debugfs entry to aide users trying to figure out why chips disappear or behave erratically. I'm ok with it being turned on but I'm not ok with a debugging step being "oh yeah, so just go comment this out and recompile", especially if people are debugging on an embedded target where it's not easily self hosted or easy to rebuild things. -adrian
>> Yes this is WIFI chip set level power-save(based on idleness) and not >> related to protocol power save. FW will turn off/scale down the >> resources(clock/rails) based on opportunity(when ever idle mode is >> detected). This power save is mostly done in disconnected state. I am >> not really sure when framework/user-space triggers power-save >> config(iw dev <devname> set power_save on/off). Then doing this from >> user-space will be unnecessarily toggling this config or may not be >> setting at disconnected state. > I think that 'set power_save' commands affects struct > ieee80211_bss_conf::ps parameter and I don't think it should be used in > this case. We already have ath10k_config_ps() for 802.11 level of power > saving. > > Arend again proposed runtime-pm with which I'm not very familiar. But > why would we want to disable this? Doesn't it make sense to have this > feature always enabled to save power? wcn3990 is a chip for a mobile > device anyway. it might be made for mobile devices but who knows how it is used by the market. Sebastian >
On 4/20/2018 9:21 AM, Sebastian Gottschall wrote: > >>> Yes this is WIFI chip set level power-save(based on idleness) and not >>> related to protocol power save. FW will turn off/scale down the >>> resources(clock/rails) based on opportunity(when ever idle mode is >>> detected). This power save is mostly done in disconnected state. I am >>> not really sure when framework/user-space triggers power-save >>> config(iw dev <devname> set power_save on/off). Then doing this from >>> user-space will be unnecessarily toggling this config or may not be >>> setting at disconnected state. >> I think that 'set power_save' commands affects struct >> ieee80211_bss_conf::ps parameter and I don't think it should be used in >> this case. We already have ath10k_config_ps() for 802.11 level of power >> saving. >> >> Arend again proposed runtime-pm with which I'm not very familiar. But >> why would we want to disable this? Doesn't it make sense to have this >> feature always enabled to save power? wcn3990 is a chip for a mobile >> device anyway. > it might be made for mobile devices but who knows how it is used by the > market. Reading the explanation above it does not make sense to use runtime-pm. That would only come into play if the host driver would actually control the resources being turned off/scaled down. So this is purely reducing power-consumption of the chip. However, it would be good to know the consequences in terms of responsiveness to firmware commands for instance when requesting a scan operation. Another thing to consider is to provide user-space with possibility to change this configuration (maybe through debugfs?). Regards, Arend
On 2018-04-20 13:46, Arend van Spriel wrote: > On 4/20/2018 9:21 AM, Sebastian Gottschall wrote: >> >>>> Yes this is WIFI chip set level power-save(based on idleness) and >>>> not >>>> related to protocol power save. FW will turn off/scale down the >>>> resources(clock/rails) based on opportunity(when ever idle mode is >>>> detected). This power save is mostly done in disconnected state. I >>>> am >>>> not really sure when framework/user-space triggers power-save >>>> config(iw dev <devname> set power_save on/off). Then doing this from >>>> user-space will be unnecessarily toggling this config or may not be >>>> setting at disconnected state. >>> I think that 'set power_save' commands affects struct >>> ieee80211_bss_conf::ps parameter and I don't think it should be used >>> in >>> this case. We already have ath10k_config_ps() for 802.11 level of >>> power >>> saving. >>> >>> Arend again proposed runtime-pm with which I'm not very familiar. But >>> why would we want to disable this? Doesn't it make sense to have this >>> feature always enabled to save power? wcn3990 is a chip for a mobile >>> device anyway. >> it might be made for mobile devices but who knows how it is used by >> the >> market. I guess having this enabled by default is safe as WMI_PDEV_PARAM_UNSUPPORTED protects for those version which does not support this pdev param. > Reading the explanation above it does not make sense to use > runtime-pm. That would only come into play if the host driver would > actually control the resources being turned off/scaled down. > > So this is purely reducing power-consumption of the chip. However, it > would be good to know the consequences in terms of responsiveness to > firmware commands for instance when requesting a scan operation. Exit latency is around 8-10 ms, so i guess this delta should be ok. > Another thing to consider is to provide user-space with possibility to > change this configuration (maybe through debugfs?). In case any one wants to measure power no's with/without this config, just giving provision to disable may be useful. BR, Govind
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > On 4/20/2018 9:21 AM, Sebastian Gottschall wrote: >> >>>> Yes this is WIFI chip set level power-save(based on idleness) and not >>>> related to protocol power save. FW will turn off/scale down the >>>> resources(clock/rails) based on opportunity(when ever idle mode is >>>> detected). This power save is mostly done in disconnected state. I am >>>> not really sure when framework/user-space triggers power-save >>>> config(iw dev <devname> set power_save on/off). Then doing this from >>>> user-space will be unnecessarily toggling this config or may not be >>>> setting at disconnected state. >>> I think that 'set power_save' commands affects struct >>> ieee80211_bss_conf::ps parameter and I don't think it should be used in >>> this case. We already have ath10k_config_ps() for 802.11 level of power >>> saving. >>> >>> Arend again proposed runtime-pm with which I'm not very familiar. But >>> why would we want to disable this? Doesn't it make sense to have this >>> feature always enabled to save power? wcn3990 is a chip for a mobile >>> device anyway. >> >> it might be made for mobile devices but who knows how it is used by the >> market. Sure, and this does parameter prevent that. You can still connect wcn3990 to any power source you want, but the driver is just optimised power consumption in focus. > Reading the explanation above it does not make sense to use > runtime-pm. That would only come into play if the host driver would > actually control the resources being turned off/scaled down. Exactly. > So this is purely reducing power-consumption of the chip. However, it > would be good to know the consequences in terms of responsiveness to > firmware commands for instance when requesting a scan operation. > Another thing to consider is to provide user-space with possibility to > change this configuration (maybe through debugfs?). Adrian was also commenting something similar about adding a debugfs interface but I don't really see the point right now while we are adding initial wcn3990 support. If someone wants to run measurements with and without this parameter it's very easy to edit it out from the sources. Later on we can consider making it dynamic if there's really the need for that, but I'm very skeptic that anyone would even need that.
On 23 April 2018 at 08:50, Kalle Valo <kvalo@codeaurora.org> wrote: > > Adrian was also commenting something similar about adding a debugfs > interface but I don't really see the point right now while we are adding > initial wcn3990 support. If someone wants to run measurements with and > without this parameter it's very easy to edit it out from the sources. > Later on we can consider making it dynamic if there's really the need > for that, but I'm very skeptic that anyone would even need that. Hi! We have at ${WORK}, because we've found various devices (atheros and qualcomm and QCA and otherwise) have this nasty habit of trying to guess when a good time to clock-gate down is, but we're busy doing low latency audio/video. 10, 20ms may not be a lot of latency for bulk data but that's a half to one of a video frame in my world. :-) -adrian
Adrian Chadd <adrian@freebsd.org> writes: > On 23 April 2018 at 08:50, Kalle Valo <kvalo@codeaurora.org> wrote: > >> >> Adrian was also commenting something similar about adding a debugfs >> interface but I don't really see the point right now while we are adding >> initial wcn3990 support. If someone wants to run measurements with and >> without this parameter it's very easy to edit it out from the sources. >> Later on we can consider making it dynamic if there's really the need >> for that, but I'm very skeptic that anyone would even need that. > > Hi! > > We have at ${WORK}, because we've found various devices (atheros and > qualcomm and QCA and otherwise) have this nasty habit of trying to > guess when a good time to clock-gate down is, but we're busy doing low > latency audio/video. > > 10, 20ms may not be a lot of latency for bulk data but that's a half > to one of a video frame in my world. :-) Sure, I get that. But we should discuss making a setting like this dynamic once we have a test case and numbers showing that it's needed, otherwise it's just unnecessary cruft.
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index 7e02ca02b28e..1d9222af1bb2 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -4679,6 +4679,13 @@ static int ath10k_start(struct ieee80211_hw *hw) } } + param = ar->wmi.pdev_param->idle_ps_config; + ret = ath10k_wmi_pdev_set_param(ar, param, 1); + if (ret && ret != -EOPNOTSUPP) { + ath10k_warn(ar, "failed to enable idle_ps_config: %d\n", ret); + goto err_core_stop; + } + __ath10k_set_antenna(ar, ar->cfg_tx_chainmask, ar->cfg_rx_chainmask); /*