On Fri, 28 Dec 2018 at 20:59, Tony Lindgren <tony@xxxxxxxxxxx> wrote: > > Hi, > > * Ulf Hansson <ulf.hansson@xxxxxxxxxx> [181228 11:01]: > > On Sun, 23 Dec 2018 at 17:02, Tony Lindgren <tony@xxxxxxxxxxx> wrote: > > > > > > Hi, > > > > > > * Reizer, Eyal <eyalr@xxxxxx> [181223 07:38]: > > > > You are ok as long as the wlan_enable pin Does go down for a sufficient amount of time > > > > turning the wl18xx device off. > > > > The firmware can only be downloaded once after power on. > > > > In between down/up you have to make sure the wlan_enable is fully going through on->off->on > > > > and the wl18xx device is fully reset. > > > > On power on the firmware is loaded by the driver and this will fail in case the reset didn't happen > > > > > > OK thanks for confirming that it's the enable pin that needs to toggle. > > > > > > Sounds like I need to get the wlcore pwrseq changes done and posted as > > > the long term solution. > > > > Just to make sure we are talking about the same things here. The > > pwrseq thingy, is already implemented in the mmc core. When it comes > > to Hikey, there is already a pwrseq node deployed in the DTB. So, we > > should be fine. > > > > Here are the MMC pwrseq bindings: > > Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt > > Documentation/devicetree/bindings/mmc/mmc.txt > > > > Here is the Hikey DTS file: > > arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts > > Oh so it seems. And looking at my earlier pwrseq_wlcore.c hacks, I only > added pinctrl handling to work around a GPIO glitch on some SoCs for > deeper idle states. So I don't think we need a pwrseq_wlcore.c as > I've found a better way to deal with the GPIO glitch by implementing > gpiochip for the pinctrl driver. > If you have a "default" pinctrl state defined in the mmc-pwrseq node that state will be set during probe. That's because we have made each pwrseq provider being a standalone driver, thus pinctrl_bind_pins() get called by driver core. I realize that we should update the DT bindings for mmc pwrseq to reflect that, I will send a patch for that. If you need some additional pinctrl states for the mmc pwrseq_simple driver, for example, we can easily add that. I assume putting the pins in a sleep state once powering off the wifi chip could make sense. However, if this is about an out-of-band IRQ line, instead of using the SDIO IRQs on DAT1, I think management of that, belongs in the wlan (or gpiochip) layers. > > > And for a short term fix, we should just add msleep(50) for now with > > > updated patch description. > > > > > > Does that that sound OK to everybody? > > > > Unfortunate, that doesn't work. Because, if user space via sysfs has > > prevented runtime suspend, the SDIO card never becomes power off with > > a call to pm_runtime_put*(), not even after waiting 50 ms. > > Yes you're right. > > > If we need a short term solution, I think there are two options. > > > > 1) Call pm_runtime_force_suspend() instead of pm_runtime_put*() at > > "down". This makes sure the card becomes powered off. At "up", call > > pm_runtime_force_resume() instead of pm_runtime_get_sync(). Because > > the runtime PM usage count it > 1, at "up", pm_runtime_force_resume() > > will power up the card, rather than deferring it. > > > > The problem with 1), is if there is an ACPI PM domain attached to the > > SDIO card device. Then this doesn't work. I can't tell if that is ever > > the case here. > > Hmm.. > > > 2) At "up", after the call to pm_runtime_get_sync(), add a call to > > mmc_hw_reset(). This forces the mmc core to power cycle and re-init > > the SDIO card. This may in some cases be a waste, as the card may > > already have been power cycled, but at least we should now always be > > able to re-program the firmware. > > Option 2 sounds more generic to me. Do you have some test patch for > this? I can send a formal patch, but not sure which of the option to pick yet, (if any). > > Sounds like you're thinking about adding this to the MMC framework? Both 1) and 2) is doable already, without having to change the MMC framework. > > If the only issue is if the card must be powered off when ifconfig > wlan0 returns, I think that's a cosmetic issue. For example, > switching to wlcore test mode after ifconfig wlan0 down might need > the card powered down, but then again the test mode resets things > anyway. Eyal? I am not convinced, sorry. We have to distinguish if "down" has a strict requirement to power off the wlan-chip. For example, in "flight mode", is it okay to leave the wlan chip powered on and thus potentially also having its radio part active? > > > If there is no rush, I think we may consider to move away from using > > runtime PM to control power for SDIO cards. Because, what we seem to > > need, is a simple interface (reference counted) to power-on/off SDIO > > cards. > > Well we should fix the regression in a minimal way first though. > And to me it sounds like option 2 above should do the trick, not > sure if we need something beyond that. Okay, let's focus on fixing the regression first, then we can look into more long term improvements. Kind regards Uffe
