| Message ID | 20210315132501.441681-9-Jerome.Pouiller@silabs.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wfx: get out from the staging area | expand |
On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller <Jerome.Pouiller@silabs.com> wrote: > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > --- > 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. [...] Kind regards Uffe
Hello Ulf, On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote: > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller > <Jerome.Pouiller@silabs.com> wrote: > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > --- > > 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 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? This behavior sounds correct to me. You would expect something more?
On Mon, 22 Mar 2021 at 18:14, Jérôme Pouiller <jerome.pouiller@silabs.com> wrote: > > Hello Ulf, > > On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote: > > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller > > <Jerome.Pouiller@silabs.com> wrote: > > > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > --- > > > 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. > > This behavior sounds correct to me. You would expect something > more? Yes, see above. Kind regards Uffe
On Tuesday 23 March 2021 15:11:56 CET Ulf Hansson wrote: > On Mon, 22 Mar 2021 at 18:14, Jérôme Pouiller <jerome.pouiller@silabs.com> wrote: > > On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote: > > > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller <Jerome.Pouiller@silabs.com> wrote: > > > > > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > --- > > > > 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.
On Tue, 23 Mar 2021 at 18:53, Jérôme Pouiller <jerome.pouiller@silabs.com> 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 <jerome.pouiller@silabs.com> wrote: > > > On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote: > > > > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller <Jerome.Pouiller@silabs.com> wrote: > > > > > > > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > --- > > > > > 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
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.
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. :-) 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
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; }
On Fri, 1 Oct 2021 at 14:31, Kalle Valo <kvalo@codeaurora.org> wrote: > > 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; Just to confirm, the code looks reasonable to me, even if the comment above looks a bit odd/outdated. :-) *) Because the SDIO driver's ->suspend|resume() callbacks have been assigned, the mmc core will not remove the corresponding SDIO func/card's struct device. **) If system wakeup *isn't* going to be enabled, the early return with 0, will allow the mmc core to power off the SDIO card/func device during system suspend. Vice versa, it will then restore power to it and re-initialize it during system resume. ***) If system wakeup *is* going to be enabled, MMC_PM_KEEP_POWER flag will prevent the mmc core from powering off the SDIO card/func device during system suspend. Depending on if the wakeup irq is in-band or out-band, MMC_PM_WAKE_SDIO_IRQ could be set too. That said, note that ->probe() of the SDIO func driver, will not be called for a non-removable SDIO func/card to re-program the FW after a system suspend/resume. That needs to be managed from the SDIO func driver's system resume callback - or deferring to upper common network layers (interface up?), which seems to be the case here. Kind regards Uffe
diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c b/drivers/net/wireless/silabs/wfx/bus_sdio.c new file mode 100644 index 000000000000..55b6728416ba --- /dev/null +++ b/drivers/net/wireless/silabs/wfx/bus_sdio.c @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * SDIO interface. + * + * Copyright (c) 2017-2020, Silicon Laboratories, Inc. + * Copyright (c) 2010, ST-Ericsson + */ +#include <linux/module.h> +#include <linux/mmc/sdio.h> +#include <linux/mmc/sdio_func.h> +#include <linux/mmc/sdio_ids.h> +#include <linux/mmc/card.h> +#include <linux/interrupt.h> +#include <linux/of_irq.h> +#include <linux/irq.h> + +#include "bus.h" +#include "wfx.h" +#include "hwio.h" +#include "main.h" +#include "bh.h" + +static const struct wfx_platform_data wfx_sdio_pdata = { + .file_fw = "wfm_wf200", + .file_pds = "wf200.pds", +}; + +struct wfx_sdio_priv { + struct sdio_func *func; + struct wfx_dev *core; + u8 buf_id_tx; + u8 buf_id_rx; + int of_irq; +}; + +static int wfx_sdio_copy_from_io(void *priv, unsigned int reg_id, + void *dst, size_t count) +{ + struct wfx_sdio_priv *bus = priv; + unsigned int sdio_addr = reg_id << 2; + int ret; + + WARN(reg_id > 7, "chip only has 7 registers"); + WARN(((uintptr_t)dst) & 3, "unaligned buffer size"); + WARN(count & 3, "unaligned buffer address"); + + /* Use queue mode buffers */ + if (reg_id == WFX_REG_IN_OUT_QUEUE) + sdio_addr |= (bus->buf_id_rx + 1) << 7; + ret = sdio_memcpy_fromio(bus->func, dst, sdio_addr, count); + if (!ret && reg_id == WFX_REG_IN_OUT_QUEUE) + bus->buf_id_rx = (bus->buf_id_rx + 1) % 4; + + return ret; +} + +static int wfx_sdio_copy_to_io(void *priv, unsigned int reg_id, + const void *src, size_t count) +{ + struct wfx_sdio_priv *bus = priv; + unsigned int sdio_addr = reg_id << 2; + int ret; + + WARN(reg_id > 7, "chip only has 7 registers"); + WARN(((uintptr_t)src) & 3, "unaligned buffer size"); + WARN(count & 3, "unaligned buffer address"); + + /* Use queue mode buffers */ + if (reg_id == WFX_REG_IN_OUT_QUEUE) + sdio_addr |= bus->buf_id_tx << 7; + /* FIXME: discards 'const' qualifier for src */ + ret = sdio_memcpy_toio(bus->func, sdio_addr, (void *)src, count); + if (!ret && reg_id == WFX_REG_IN_OUT_QUEUE) + bus->buf_id_tx = (bus->buf_id_tx + 1) % 32; + + return ret; +} + +static void wfx_sdio_lock(void *priv) +{ + struct wfx_sdio_priv *bus = priv; + + sdio_claim_host(bus->func); +} + +static void wfx_sdio_unlock(void *priv) +{ + struct wfx_sdio_priv *bus = priv; + + sdio_release_host(bus->func); +} + +static void wfx_sdio_irq_handler(struct sdio_func *func) +{ + struct wfx_sdio_priv *bus = sdio_get_drvdata(func); + + wfx_bh_request_rx(bus->core); +} + +static irqreturn_t wfx_sdio_irq_handler_ext(int irq, void *priv) +{ + struct wfx_sdio_priv *bus = priv; + + sdio_claim_host(bus->func); + wfx_bh_request_rx(bus->core); + sdio_release_host(bus->func); + return IRQ_HANDLED; +} + +static int wfx_sdio_irq_subscribe(void *priv) +{ + struct wfx_sdio_priv *bus = priv; + u32 flags; + int ret; + u8 cccr; + + if (!bus->of_irq) { + sdio_claim_host(bus->func); + ret = sdio_claim_irq(bus->func, wfx_sdio_irq_handler); + sdio_release_host(bus->func); + return ret; + } + + sdio_claim_host(bus->func); + cccr = sdio_f0_readb(bus->func, SDIO_CCCR_IENx, NULL); + cccr |= BIT(0); + cccr |= BIT(bus->func->num); + sdio_f0_writeb(bus->func, cccr, SDIO_CCCR_IENx, NULL); + sdio_release_host(bus->func); + flags = irq_get_trigger_type(bus->of_irq); + if (!flags) + flags = IRQF_TRIGGER_HIGH; + flags |= IRQF_ONESHOT; + return devm_request_threaded_irq(&bus->func->dev, bus->of_irq, NULL, + wfx_sdio_irq_handler_ext, flags, + "wfx", bus); +} + +static int wfx_sdio_irq_unsubscribe(void *priv) +{ + struct wfx_sdio_priv *bus = priv; + int ret; + + if (bus->of_irq) + devm_free_irq(&bus->func->dev, bus->of_irq, bus); + sdio_claim_host(bus->func); + ret = sdio_release_irq(bus->func); + sdio_release_host(bus->func); + return ret; +} + +static size_t wfx_sdio_align_size(void *priv, size_t size) +{ + struct wfx_sdio_priv *bus = priv; + + return sdio_align_size(bus->func, size); +} + +static const struct hwbus_ops wfx_sdio_hwbus_ops = { + .copy_from_io = wfx_sdio_copy_from_io, + .copy_to_io = wfx_sdio_copy_to_io, + .irq_subscribe = wfx_sdio_irq_subscribe, + .irq_unsubscribe = wfx_sdio_irq_unsubscribe, + .lock = wfx_sdio_lock, + .unlock = wfx_sdio_unlock, + .align_size = wfx_sdio_align_size, +}; + +static const struct of_device_id wfx_sdio_of_match[] = { + { .compatible = "silabs,wfx-sdio" }, + { .compatible = "silabs,wf200" }, + { }, +}; +MODULE_DEVICE_TABLE(of, wfx_sdio_of_match); + +static int wfx_sdio_probe(struct sdio_func *func, + const struct sdio_device_id *id) +{ + struct device_node *np = func->dev.of_node; + struct wfx_sdio_priv *bus; + int ret; + + if (func->num != 1) { + dev_err(&func->dev, "SDIO function number is %d while it should always be 1 (unsupported chip?)\n", + func->num); + return -ENODEV; + } + + bus = devm_kzalloc(&func->dev, sizeof(*bus), GFP_KERNEL); + if (!bus) + return -ENOMEM; + + if (!np || !of_match_node(wfx_sdio_of_match, np)) { + dev_warn(&func->dev, "no compatible device found in DT\n"); + return -ENODEV; + } + + bus->func = func; + bus->of_irq = irq_of_parse_and_map(np, 0); + sdio_set_drvdata(func, bus); + func->card->quirks |= MMC_QUIRK_LENIENT_FN0 | + MMC_QUIRK_BLKSZ_FOR_BYTE_MODE | + MMC_QUIRK_BROKEN_BYTE_MODE_512; + + sdio_claim_host(func); + ret = sdio_enable_func(func); + /* Block of 64 bytes is more efficient than 512B for frame sizes < 4k */ + sdio_set_block_size(func, 64); + sdio_release_host(func); + if (ret) + goto err0; + + bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata, + &wfx_sdio_hwbus_ops, bus); + if (!bus->core) { + ret = -EIO; + goto err1; + } + + ret = wfx_probe(bus->core); + if (ret) + goto err1; + + return 0; + +err1: + sdio_claim_host(func); + sdio_disable_func(func); + sdio_release_host(func); +err0: + return ret; +} + +static void wfx_sdio_remove(struct sdio_func *func) +{ + struct wfx_sdio_priv *bus = sdio_get_drvdata(func); + + wfx_release(bus->core); + sdio_claim_host(func); + sdio_disable_func(func); + sdio_release_host(func); +} + +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, + } +};