Re: [PATCH v5 08/24] wfx: add bus_sdio.c

[Kalle Valo <kvalo@codeaurora.org>]

Hi Ulf,

sorry for the late reply, my Gnus tells me it took me 24 weeks to reply :)

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Wed, 7 Apr 2021 at 14:00, Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>> >> If I follow what has been done in other drivers I would write something
>> >> like:
>> >>
>> >>   static int wfx_sdio_suspend(struct device *dev)
>> >>   {
>> >>           struct sdio_func *func = dev_to_sdio_func(dev);
>> >>           struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
>> >>
>> >>           config_reg_write_bits(bus->core, CFG_IRQ_ENABLE_DATA, 0);
>> >>           // Necessary to keep device firmware in RAM
>> >>           return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>> >
>> > This will tell the mmc/sdio core to keep the SDIO card powered on
>> > during system suspend. Thus, it doesn't need to re-initialize it at
>> > system resume - and the firmware should not need to be re-programmed.
>> >
>> > On the other hand, if you don't plan to support system wakeups, it
>> > would probably be better to power off the card, to avoid wasting
>> > energy while the system is suspended. I assume that means you need to
>> > re-program the firmware as well. Normally, it's these kinds of things
>> > that need to be managed from a ->resume() callback.
>>
>> Many mac80211 drivers do so that the device is powered off during
>> interface down (ifconfig wlan0 down), and as mac80211 does interface
>> down automatically during suspend, suspend then works without extra
>> handlers.
>
> That sounds simple. :-)

Indeed, I was omitting a lot of details :) My comment was more like a
general remark to all different bus techonologies, not just about SDIO.
And I'm not saying that all wireless drivers do that, but some of them
do. Though I don't have any numbers how many.

> Would you mind elaborating on what is actually being powered off at
> interface down - and thus also I am curious what happens at a typical
> interface up?

In general in the drivers that do we this the firmware is completely
turned off and all memory is reset during interface down. And firmware
is started from the scratch during interface up. Also one benefit from
this is that firmware state is reset, the wireless firmwares are
notarious being buggy.

> Even if we don't want to use system wakeups (wake-on-lan), the SDIO
> core and the SDIO func driver still need to somewhat agree on how to
> manage the power for the card during system suspend, I think.
>
> For example, for a non-removable SDIO card, the SDIO/MMC core may
> decide to power off the card in system suspend. Then it needs to
> restore power to the card and re-initialize it at system resume, of
> course. This doesn't mean that the actual corresponding struct device
> for it, gets removed/re-added, thus the SDIO func driver isn't being
> re-probed after the system has resumed. Although, since the SDIO card
> was re-initialized, it's likely that the FW may need to be
> re-programmed after the system has been resumed.
>
> Are you saying that re-programming the FW is always happening at
> interface up, when there are none system suspend/resume callbacks
> assigned for the SDIO func driver?

Yes, that's what I was trying to say. But take all this with grain of
salt, I'm not very familiar with SDIO! And funnily enough, I checked
what we do in ath10k_sdio driver during suspend has conflicting code and
documentation:

/* Empty handlers so that mmc subsystem doesn't remove us entirely during
 * suspend. We instead follow cfg80211 suspend/resume handlers.
 */
static int ath10k_sdio_pm_suspend(struct device *device)
{
	struct sdio_func *func = dev_to_sdio_func(device);
	struct ath10k_sdio *ar_sdio = sdio_get_drvdata(func);
	struct ath10k *ar = ar_sdio->ar;
	mmc_pm_flag_t pm_flag, pm_caps;
	int ret;

	if (!device_may_wakeup(ar->dev))
		return 0;

	ath10k_sdio_set_mbox_sleep(ar, true);

	pm_flag = MMC_PM_KEEP_POWER;

	ret = sdio_set_host_pm_flags(func, pm_flag);
	if (ret) {
		pm_caps = sdio_get_host_pm_caps(func);
		ath10k_warn(ar, "failed to set sdio host pm flags (0x%x, 0x%x): %d\n",
			    pm_flag, pm_caps, ret);
		return ret;
	}

	return ret;
}

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches