MIME-Version: 1.0
References: <20181217164207.20081-1-tony@atomide.com> <CAPDyKFoLnNWsBfaUwqwL31J=zb0UOQjXUKi7Sj0AUsnNoC6bYg@mail.gmail.com>
 <20181218155439.GB6707@atomide.com> <CAPDyKFr2xx6fmdgjd10C1jekWg-yZPk32fGWEPO2PyexjofJ1A@mail.gmail.com>
 <20181220231401.GG6707@atomide.com> <c22e2ab61b494ea0852c0f4654fef92d@ti.com>
 <20181223160226.GK6707@atomide.com> <CAPDyKFoEOGACs4Wnm7UgKab2b1-7+v8+31DpC+p=8XxcQhSpcQ@mail.gmail.com>
 <20181228195903.GX6707@atomide.com>
In-Reply-To: <20181228195903.GX6707@atomide.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 2 Jan 2019 13:01:50 +0100
Message-ID: <CAPDyKFq9XmRueJEFKVb03hqLuoB84TGjrdvEjre46ezYbk_wgw@mail.gmail.com>
Subject: Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if
 powered down briefly
To:     Tony Lindgren <tony@atomide.com>
Cc:     "Reizer, Eyal" <eyalr@ti.com>, Kalle Valo <kvalo@codeaurora.org>,
        KISHON VIJAY ABRAHAM <kishon@ti.com>,
        "Mishol, Guy" <guym@ti.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        linux-omap <linux-omap@vger.kernel.org>,
        Anders Roxell <anders.roxell@linaro.org>,
        John Stultz <john.stultz@linaro.org>,
        Ricardo Salveti <rsalveti@rsalveti.net>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Fri, 28 Dec 2018 at 20:59, Tony Lindgren <tony@atomide.com> wrote:
>
> Hi,
>
> * Ulf Hansson <ulf.hansson@linaro.org> [181228 11:01]:
> > On Sun, 23 Dec 2018 at 17:02, Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > Hi,
> > >
> > > * Reizer, Eyal <eyalr@ti.com> [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