Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Arend van Spriel]

- 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

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Ulf Hansson]

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

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Arend van Spriel]

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

Re: [PATCH 07/10] brcmfmac: fix sdio suspend and resume

[Ulf Hansson]

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