2021-03-23 17:55:43

by Jérôme Pouiller

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

On Tuesday 23 March 2021 15:11:56 CET Ulf Hansson wrote:
> On Mon, 22 Mar 2021 at 18:14, J?r?me Pouiller <[email protected]> wrote:
> > On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote:
> > > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller <[email protected]> wrote:
> > > >
> > > > From: J?r?me Pouiller <[email protected]>
> > > >
> > > > Signed-off-by: J?r?me Pouiller <[email protected]>
> > > > ---
> > > > drivers/net/wireless/silabs/wfx/bus_sdio.c | 259 +++++++++++++++++++++
> > > > 1 file changed, 259 insertions(+)
> > > > create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > >
> > > [...]
> > >
> > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> > > > + { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> > > > +
> > > > +struct sdio_driver wfx_sdio_driver = {
> > > > + .name = "wfx-sdio",
> > > > + .id_table = wfx_sdio_ids,
> > > > + .probe = wfx_sdio_probe,
> > > > + .remove = wfx_sdio_remove,
> > > > + .drv = {
> > > > + .owner = THIS_MODULE,
> > > > + .of_match_table = wfx_sdio_of_match,
> > >
> > > It's not mandatory to support power management, like system
> > > suspend/resume. However, as this looks like this is a driver for an
> > > embedded SDIO device, you probably want this.
> > >
> > > If that is the case, please assign the dev_pm_ops here and implement
> > > the ->suspend|resume() callbacks.
> >
> > I have no platform to test suspend/resume, so I have only a
> > theoretical understanding of this subject.
>
> I see.
>
> >
> > I understanding is that with the current implementation, the
> > device will be powered off on suspend and then totally reset
> > (including reloading of the firmware) on resume. I am wrong?
>
> You are correct, for a *removable* SDIO card. In this case, the
> mmc/sdio core will remove the corresponding SDIO card/device and its
> corresponding SDIO func devices at system suspend. It will then be
> redetected at system resume (and the SDIO func driver re-probed).
>
> Although, as this is an embedded SDIO device, per definition it's not
> a removable card (MMC_CAP_NONREMOVABLE should be set for the
> corresponding mmc host), the SDIO card will stick around and instead
> the ->suspend|resume() callback needs to be implemented for the SDIO
> func driver.

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);
}

However, why not the implementation below?

static int wfx_sdio_suspend(struct device *dev)
{
struct sdio_func *func = dev_to_sdio_func(dev);

wfx_sdio_remove(func);
return 0;
}

In both cases, I worry to provide these functions without being able to
test them.


--
J?r?me Pouiller



2021-03-23 19:15:09

by Ulf Hansson

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

On Tue, 23 Mar 2021 at 18:53, Jérôme Pouiller
<[email protected]> wrote:
>
> On Tuesday 23 March 2021 15:11:56 CET Ulf Hansson wrote:
> > On Mon, 22 Mar 2021 at 18:14, Jérôme Pouiller <[email protected]> wrote:
> > > On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote:
> > > > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller <[email protected]> wrote:
> > > > >
> > > > > From: Jérôme Pouiller <[email protected]>
> > > > >
> > > > > Signed-off-by: Jérôme Pouiller <[email protected]>
> > > > > ---
> > > > > drivers/net/wireless/silabs/wfx/bus_sdio.c | 259 +++++++++++++++++++++
> > > > > 1 file changed, 259 insertions(+)
> > > > > create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > > >
> > > > [...]
> > > >
> > > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> > > > > + { },
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> > > > > +
> > > > > +struct sdio_driver wfx_sdio_driver = {
> > > > > + .name = "wfx-sdio",
> > > > > + .id_table = wfx_sdio_ids,
> > > > > + .probe = wfx_sdio_probe,
> > > > > + .remove = wfx_sdio_remove,
> > > > > + .drv = {
> > > > > + .owner = THIS_MODULE,
> > > > > + .of_match_table = wfx_sdio_of_match,
> > > >
> > > > It's not mandatory to support power management, like system
> > > > suspend/resume. However, as this looks like this is a driver for an
> > > > embedded SDIO device, you probably want this.
> > > >
> > > > If that is the case, please assign the dev_pm_ops here and implement
> > > > the ->suspend|resume() callbacks.
> > >
> > > I have no platform to test suspend/resume, so I have only a
> > > theoretical understanding of this subject.
> >
> > I see.
> >
> > >
> > > I understanding is that with the current implementation, the
> > > device will be powered off on suspend and then totally reset
> > > (including reloading of the firmware) on resume. I am wrong?
> >
> > You are correct, for a *removable* SDIO card. In this case, the
> > mmc/sdio core will remove the corresponding SDIO card/device and its
> > corresponding SDIO func devices at system suspend. It will then be
> > redetected at system resume (and the SDIO func driver re-probed).
> >
> > Although, as this is an embedded SDIO device, per definition it's not
> > a removable card (MMC_CAP_NONREMOVABLE should be set for the
> > corresponding mmc host), the SDIO card will stick around and instead
> > the ->suspend|resume() callback needs to be implemented for the SDIO
> > func driver.
>
> 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.

> }
>
> However, why not the implementation below?
>
> static int wfx_sdio_suspend(struct device *dev)
> {
> struct sdio_func *func = dev_to_sdio_func(dev);
>
> wfx_sdio_remove(func);

I don't know what wfx_sdio_remove() does, but for sure you would need
a ->resume() callback to make it possible to restore power/firmware.

> return 0;
> }
>
> In both cases, I worry to provide these functions without being able to
> test them.

Alright, let's simply leave this driver without having the PM
callbacks assigned. I guess we can revisit this at some later point.

The mmc core will log a message about the missing callbacks, in case
someone tries to execute system suspend/resume when the driver has
been probed.

Kind regards
Uffe

2021-04-07 20:55:38

by Kalle Valo

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

Ulf Hansson <[email protected]> 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.

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

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

2021-04-12 08:24:56

by Ulf Hansson

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

On Wed, 7 Apr 2021 at 14:00, Kalle Valo <[email protected]> wrote:
>
> Ulf Hansson <[email protected]> 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. :-)

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?

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?

Kind regards
Uffe