* Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume [not found] ` <CAPDyKFqGb81m9ExECL1ZCea8OuhekH+aWiXC=8zafnLN2T02Vw@mail.gmail.com> @ 2015-04-22 8:52 ` Arend van Spriel 2015-04-22 9:18 ` Ulf Hansson 0 siblings, 1 reply; 4+ messages in thread From: Arend van Spriel @ 2015-04-22 8:52 UTC (permalink / raw) To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter - wireless list/maintainer On 04/22/15 09:32, Ulf Hansson wrote: > On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com> wrote: >> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.") >> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for >> non-wowl scenario, which needs to be restored. Another necessary >> change is to mark the card as being non-removable. With this in place >> the suspend resume test passes successfully doing: >> >> # echo devices> /sys/power/pm_test >> # echo mem> /sys/power/state >> >> Note that power may still be switched off when system is going >> in S3 state. >> >> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com> >> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com> >> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com> >> Signed-off-by: Arend van Spriel<arend@broadcom.com> >> --- >> drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >> index 9b508bd..8a69544 100644 >> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev) >> return 0; >> } >> >> +static void brcmf_sdiod_host_fixup(struct mmc_host *host) >> +{ >> + /* runtime-pm powers off the device */ >> + pm_runtime_forbid(host->parent); > > That you need this, clearly shows that something is broken in the mmc > core/host layer. This patch only moved this. The patch introducing this is here [1]. > Could you elaborate a bit on what configuration you are using. Like > what mmc host, which SDIO bus speed mode. Not just one. My test setup is a dev board hooked up to a card reader slot using sdhci-pci driver. Another setup I have is a chromebook with our device integrated with dw_mmc-rockchip driver. It is an arm platform with dt entry: &sdio0 { broken-cd; bus-width = <4>; cap-sd-highspeed; sd-uhs-sdr12; sd-uhs-sdr25; sd-uhs-sdr50; sd-uhs-sdr104; cap-sdio-irq; card-external-vcc-supply = <&wifi_regulator>; clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>, <&cru SCLK_SDIO0_DRV>, <&cru SCLK_SDIO0_SAMPLE>, <&rk808 RK808_CLKOUT1>; clock-names = "biu", "ciu", "ciu_drv", "ciu_sample", "card_ext_clock"; keep-power-in-suspend; non-removable; num-slots = <1>; default-sample-phase = <90>; pinctrl-names = "default"; pinctrl-0 = <&sdio0_clk &sdio0_cmd &sdio0_bus4>; status = "okay"; vmmc-supply = <&vcc33_sys>; vqmmc-supply = <&vcc18_wl>; }; I think card-external-vcc-supply is property that chromeos kernel handles to power the device. > And have you tested different configurations? Like what happens if you > use a different SDIO bus speed mode? Regards, Arend [1] https://patchwork.kernel.org/patch/6038511/ >> + /* avoid removal detection upon resume */ >> + host->caps |= MMC_CAP_NONREMOVABLE; >> +} >> + >> static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) >> { >> struct sdio_func *func; >> @@ -1076,7 +1084,7 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) >> ret = -ENODEV; >> goto out; >> } >> - pm_runtime_forbid(host->parent); >> + brcmf_sdiod_host_fixup(host); >> out: >> if (ret) >> brcmf_sdiod_remove(sdiodev); >> @@ -1246,15 +1254,15 @@ static int brcmf_ops_sdio_suspend(struct device *dev) >> brcmf_sdiod_freezer_on(sdiodev); >> brcmf_sdio_wd_timer(sdiodev->bus, 0); >> >> + sdio_flags = MMC_PM_KEEP_POWER; >> if (sdiodev->wowl_enabled) { >> - sdio_flags = MMC_PM_KEEP_POWER; >> if (sdiodev->pdata->oob_irq_supported) >> enable_irq_wake(sdiodev->pdata->oob_irq_nr); >> else >> - sdio_flags = MMC_PM_WAKE_SDIO_IRQ; >> - if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags)) >> - brcmf_err("Failed to set pm_flags %x\n", sdio_flags); >> + sdio_flags |= MMC_PM_WAKE_SDIO_IRQ; >> } >> + if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags)) >> + brcmf_err("Failed to set pm_flags %x\n", sdio_flags); >> return 0; >> } >> >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume 2015-04-22 8:52 ` [PATCH 07/10] brcmfmac: fix sdio suspend and resume Arend van Spriel @ 2015-04-22 9:18 ` Ulf Hansson 2015-04-22 9:38 ` Arend van Spriel 0 siblings, 1 reply; 4+ messages in thread From: Ulf Hansson @ 2015-04-22 9:18 UTC (permalink / raw) To: Arend van Spriel; +Cc: linux-mmc, Adrian Hunter On 22 April 2015 at 10:52, Arend van Spriel <arend@broadcom.com> wrote: > - wireless list/maintainer > > > On 04/22/15 09:32, Ulf Hansson wrote: >> >> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com> wrote: >>> >>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.") >>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for >>> non-wowl scenario, which needs to be restored. Another necessary >>> change is to mark the card as being non-removable. With this in place >>> the suspend resume test passes successfully doing: >>> >>> # echo devices> /sys/power/pm_test >>> # echo mem> /sys/power/state >>> >>> Note that power may still be switched off when system is going >>> in S3 state. >>> >>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com> >>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com> >>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com> >>> Signed-off-by: Arend van Spriel<arend@broadcom.com> >>> --- >>> drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 >>> +++++++++++++----- >>> 1 file changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>> index 9b508bd..8a69544 100644 >>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct >>> brcmf_sdio_dev *sdiodev) >>> return 0; >>> } >>> >>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host) >>> +{ >>> + /* runtime-pm powers off the device */ >>> + pm_runtime_forbid(host->parent); >> >> >> That you need this, clearly shows that something is broken in the mmc >> core/host layer. > > > This patch only moved this. The patch introducing this is here [1]. OK, thanks for sharing the information! > >> Could you elaborate a bit on what configuration you are using. Like >> what mmc host, which SDIO bus speed mode. > > > Not just one. My test setup is a dev board hooked up to a card reader slot > using sdhci-pci driver. Another setup I have is a chromebook with our device > integrated with dw_mmc-rockchip driver. It is an arm platform with dt entry: > > &sdio0 { > broken-cd; > bus-width = <4>; > cap-sd-highspeed; > sd-uhs-sdr12; > sd-uhs-sdr25; > sd-uhs-sdr50; > sd-uhs-sdr104; > cap-sdio-irq; > card-external-vcc-supply = <&wifi_regulator>; > clocks = <&cru HCLK_SDIO0>, <&cru SCLK_SDIO0>, <&cru > SCLK_SDIO0_DRV>, > <&cru SCLK_SDIO0_SAMPLE>, <&rk808 RK808_CLKOUT1>; > clock-names = "biu", "ciu", "ciu_drv", "ciu_sample", > "card_ext_clock"; > keep-power-in-suspend; > non-removable; > num-slots = <1>; > default-sample-phase = <90>; > pinctrl-names = "default"; > pinctrl-0 = <&sdio0_clk &sdio0_cmd &sdio0_bus4>; > status = "okay"; > vmmc-supply = <&vcc33_sys>; > vqmmc-supply = <&vcc18_wl>; > }; > > I think card-external-vcc-supply is property that chromeos kernel handles to > power the device. Should then likely be moved in a mmc pwrseq node and handled by the mmc core, right? The same might apply to "card_ext_clock"!? > >> And have you tested different configurations? Like what happens if you >> use a different SDIO bus speed mode? In the dw_mmc case, the host controller driver doesn't implement runtime PM - only system PM (unless you are carrying some additional out of tree patch :-) ). So, are you sure the bug exists in this configuration at all? Kind regards Uffe ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume 2015-04-22 9:18 ` Ulf Hansson @ 2015-04-22 9:38 ` Arend van Spriel 2015-04-22 13:02 ` Ulf Hansson 0 siblings, 1 reply; 4+ messages in thread From: Arend van Spriel @ 2015-04-22 9:38 UTC (permalink / raw) To: Ulf Hansson; +Cc: linux-mmc, Adrian Hunter On 04/22/15 11:18, Ulf Hansson wrote: > On 22 April 2015 at 10:52, Arend van Spriel<arend@broadcom.com> wrote: >> - wireless list/maintainer >> >> >> On 04/22/15 09:32, Ulf Hansson wrote: >>> >>> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com> wrote: >>>> >>>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.") >>>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for >>>> non-wowl scenario, which needs to be restored. Another necessary >>>> change is to mark the card as being non-removable. With this in place >>>> the suspend resume test passes successfully doing: >>>> >>>> # echo devices> /sys/power/pm_test >>>> # echo mem> /sys/power/state >>>> >>>> Note that power may still be switched off when system is going >>>> in S3 state. >>>> >>>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com> >>>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com> >>>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com> >>>> Signed-off-by: Arend van Spriel<arend@broadcom.com> >>>> --- >>>> drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 >>>> +++++++++++++----- >>>> 1 file changed, 13 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>> index 9b508bd..8a69544 100644 >>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct >>>> brcmf_sdio_dev *sdiodev) >>>> return 0; >>>> } >>>> >>>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host) >>>> +{ >>>> + /* runtime-pm powers off the device */ >>>> + pm_runtime_forbid(host->parent); >>> >>> >>> That you need this, clearly shows that something is broken in the mmc >>> core/host layer. >> >> >> This patch only moved this. The patch introducing this is here [1]. > > OK, thanks for sharing the information! > >> >>> Could you elaborate a bit on what configuration you are using. Like >>> what mmc host, which SDIO bus speed mode. >> >> >> Not just one. My test setup is a dev board hooked up to a card reader slot >> using sdhci-pci driver. Another setup I have is a chromebook with our device >> integrated with dw_mmc-rockchip driver. It is an arm platform with dt entry: >> >> &sdio0 { >> broken-cd; >> bus-width =<4>; >> cap-sd-highspeed; >> sd-uhs-sdr12; >> sd-uhs-sdr25; >> sd-uhs-sdr50; >> sd-uhs-sdr104; >> cap-sdio-irq; >> card-external-vcc-supply =<&wifi_regulator>; >> clocks =<&cru HCLK_SDIO0>,<&cru SCLK_SDIO0>,<&cru >> SCLK_SDIO0_DRV>, >> <&cru SCLK_SDIO0_SAMPLE>,<&rk808 RK808_CLKOUT1>; >> clock-names = "biu", "ciu", "ciu_drv", "ciu_sample", >> "card_ext_clock"; >> keep-power-in-suspend; >> non-removable; >> num-slots =<1>; >> default-sample-phase =<90>; >> pinctrl-names = "default"; >> pinctrl-0 =<&sdio0_clk&sdio0_cmd&sdio0_bus4>; >> status = "okay"; >> vmmc-supply =<&vcc33_sys>; >> vqmmc-supply =<&vcc18_wl>; >> }; >> >> I think card-external-vcc-supply is property that chromeos kernel handles to >> power the device. > > Should then likely be moved in a mmc pwrseq node and handled by the > mmc core, right? > > The same might apply to "card_ext_clock"!? Guess so. I was not involved in these DT changes and my focus is more on upstream. >> >>> And have you tested different configurations? Like what happens if you >>> use a different SDIO bus speed mode? > > In the dw_mmc case, the host controller driver doesn't implement > runtime PM - only system PM (unless you are carrying some additional > out of tree patch :-) ). > > So, are you sure the bug exists in this configuration at all? Forgot to mention the most applicable setup. I also have a Sony Vaio Duo 13 with wifi sdio chip integrated through sdhci-acpi driver, which is doing runtime-pm. I had a number of people contacting me and I had them disable runtime-pm through sysfs to get working wifi. That was the reason for adding pm_runtime_forbid() to brcmfmac driver. Regards, Arend ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume 2015-04-22 9:38 ` Arend van Spriel @ 2015-04-22 13:02 ` Ulf Hansson 0 siblings, 0 replies; 4+ messages in thread From: Ulf Hansson @ 2015-04-22 13:02 UTC (permalink / raw) To: Arend van Spriel; +Cc: linux-mmc, Adrian Hunter On 22 April 2015 at 11:38, Arend van Spriel <arend@broadcom.com> wrote: > On 04/22/15 11:18, Ulf Hansson wrote: >> >> On 22 April 2015 at 10:52, Arend van Spriel<arend@broadcom.com> wrote: >>> >>> - wireless list/maintainer >>> >>> >>> On 04/22/15 09:32, Ulf Hansson wrote: >>>> >>>> >>>> On 14 April 2015 at 20:10, Arend van Spriel<arend@broadcom.com> wrote: >>>>> >>>>> >>>>> commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.") >>>>> changed the behaviour by removing the MMC_PM_KEEP_POWER flag for >>>>> non-wowl scenario, which needs to be restored. Another necessary >>>>> change is to mark the card as being non-removable. With this in place >>>>> the suspend resume test passes successfully doing: >>>>> >>>>> # echo devices> /sys/power/pm_test >>>>> # echo mem> /sys/power/state >>>>> >>>>> Note that power may still be switched off when system is going >>>>> in S3 state. >>>>> >>>>> Reported-by: Fu, Zhonghui<<zhonghui.fu@linux.intel.com> >>>>> Reviewed-by: Pieter-Paul Giesberts<pieterpg@broadcom.com> >>>>> Reviewed-by: Franky (Zhenhui) Lin<frankyl@broadcom.com> >>>>> Signed-off-by: Arend van Spriel<arend@broadcom.com> >>>>> --- >>>>> drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 18 >>>>> +++++++++++++----- >>>>> 1 file changed, 13 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>>> b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>>> index 9b508bd..8a69544 100644 >>>>> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c >>>>> @@ -1011,6 +1011,14 @@ static int brcmf_sdiod_remove(struct >>>>> brcmf_sdio_dev *sdiodev) >>>>> return 0; >>>>> } >>>>> >>>>> +static void brcmf_sdiod_host_fixup(struct mmc_host *host) >>>>> +{ >>>>> + /* runtime-pm powers off the device */ >>>>> + pm_runtime_forbid(host->parent); >>>> >>>> >>>> >>>> That you need this, clearly shows that something is broken in the mmc >>>> core/host layer. >>> >>> >>> >>> This patch only moved this. The patch introducing this is here [1]. >> >> >> OK, thanks for sharing the information! >> >>> >>>> Could you elaborate a bit on what configuration you are using. Like >>>> what mmc host, which SDIO bus speed mode. >>> >>> >>> >>> Not just one. My test setup is a dev board hooked up to a card reader >>> slot >>> using sdhci-pci driver. Another setup I have is a chromebook with our >>> device >>> integrated with dw_mmc-rockchip driver. It is an arm platform with dt >>> entry: >>> >>> &sdio0 { >>> broken-cd; >>> bus-width =<4>; >>> cap-sd-highspeed; >>> sd-uhs-sdr12; >>> sd-uhs-sdr25; >>> sd-uhs-sdr50; >>> sd-uhs-sdr104; >>> cap-sdio-irq; >>> card-external-vcc-supply =<&wifi_regulator>; >>> clocks =<&cru HCLK_SDIO0>,<&cru SCLK_SDIO0>,<&cru >>> SCLK_SDIO0_DRV>, >>> <&cru SCLK_SDIO0_SAMPLE>,<&rk808 RK808_CLKOUT1>; >>> clock-names = "biu", "ciu", "ciu_drv", "ciu_sample", >>> "card_ext_clock"; >>> keep-power-in-suspend; >>> non-removable; >>> num-slots =<1>; >>> default-sample-phase =<90>; >>> pinctrl-names = "default"; >>> pinctrl-0 =<&sdio0_clk&sdio0_cmd&sdio0_bus4>; >>> status = "okay"; >>> vmmc-supply =<&vcc33_sys>; >>> vqmmc-supply =<&vcc18_wl>; >>> }; >>> >>> I think card-external-vcc-supply is property that chromeos kernel handles >>> to >>> power the device. >> >> >> Should then likely be moved in a mmc pwrseq node and handled by the >> mmc core, right? >> >> The same might apply to "card_ext_clock"!? > > > Guess so. I was not involved in these DT changes and my focus is more on > upstream. > >>> >>>> And have you tested different configurations? Like what happens if you >>>> use a different SDIO bus speed mode? >> >> >> In the dw_mmc case, the host controller driver doesn't implement >> runtime PM - only system PM (unless you are carrying some additional >> out of tree patch :-) ). >> >> So, are you sure the bug exists in this configuration at all? > > > Forgot to mention the most applicable setup. I also have a Sony Vaio Duo 13 > with wifi sdio chip integrated through sdhci-acpi driver, which is doing > runtime-pm. I had a number of people contacting me and I had them disable > runtime-pm through sysfs to get working wifi. That was the reason for adding > pm_runtime_forbid() to brcmfmac driver. Okay. Anyway, if you find time to try the other configurations and then change to "allow" runtime PM, it would be nice to know the results. Kind regards Uffe ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-04-22 13:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1429035033-14076-1-git-send-email-arend@broadcom.com>
[not found] ` <1429035033-14076-8-git-send-email-arend@broadcom.com>
[not found] ` <CAPDyKFqGb81m9ExECL1ZCea8OuhekH+aWiXC=8zafnLN2T02Vw@mail.gmail.com>
2015-04-22 8:52 ` [PATCH 07/10] brcmfmac: fix sdio suspend and resume Arend van Spriel
2015-04-22 9:18 ` Ulf Hansson
2015-04-22 9:38 ` Arend van Spriel
2015-04-22 13:02 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox