[PATCH 0/7] mmc: sdio: Various fixes/improvements for SDIO PM

[PATCH 0/7] mmc: sdio: Various fixes/improvements for SDIO PM

[Ulf Hansson]

The power management support implemented via system suspend/resume and runtime
suspend/resume for SDIO cards is rather messy, but also fragile.

This series makes some improvement to this code. In particular the so called
powered-on re-initialization of SDIO card is quite questionable, I suspect
it may never really worked well. Therefore this series removes this code, which
helps to prepare for additional improvements on top in a later series.

So far the series has only been compile tested, so any help in testing on HW for
regressions is greatly appreciated.

Kind regards
Uffe

Ulf Hansson (7):
  mmc: sdio: Turn sdio_run_irqs() into static
  mmc: sdio: Drop mmc_claim|release_host() in mmc_sdio_power_restore()
  mmc: sdio: Move comment about re-initialization to
    mmc_sdio_reinit_card()
  mmc: sdio: Drop powered-on re-init at runtime resume and HW reset
  mmc: sdio: Don't re-initialize powered-on removable SDIO cards at
    resume
  mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()
  mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()

 drivers/mmc/core/sdio.c     | 92 +++++++++++++++----------------------
 drivers/mmc/core/sdio_irq.c |  3 +-
 include/linux/mmc/host.h    |  1 -
 3 files changed, 38 insertions(+), 58 deletions(-)

-- 
2.17.1

[PATCH 1/7] mmc: sdio: Turn sdio_run_irqs() into static

[Ulf Hansson]

All external users of sdio_run_irqs() have converted into using the
preferred sdio_signal_irq() interface, thus not calling the function
directly any more. Avoid further new users of it, by turning it into
static.

Suggested-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 3 +--
 include/linux/mmc/host.h    | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 9f54a259a1b3..0bcc5e83bd1a 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -92,7 +92,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 	return ret;
 }
 
-void sdio_run_irqs(struct mmc_host *host)
+static void sdio_run_irqs(struct mmc_host *host)
 {
 	mmc_claim_host(host);
 	if (host->sdio_irqs) {
@@ -103,7 +103,6 @@ void sdio_run_irqs(struct mmc_host *host)
 	}
 	mmc_release_host(host);
 }
-EXPORT_SYMBOL_GPL(sdio_run_irqs);
 
 void sdio_irq_work(struct work_struct *work)
 {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index ecb7972e2423..a9b12322c775 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -504,7 +504,6 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 		wake_up_process(host->sdio_irq_thread);
 }
 
-void sdio_run_irqs(struct mmc_host *host);
 void sdio_signal_irq(struct mmc_host *host);
 
 #ifdef CONFIG_REGULATOR
-- 
2.17.1

Re: [PATCH 1/7] mmc: sdio: Turn sdio_run_irqs() into static

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> All external users of sdio_run_irqs() have converted into using the
> preferred sdio_signal_irq() interface, thus not calling the function
> directly any more. Avoid further new users of it, by turning it into
> static.
>
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio_irq.c | 3 +--
>  include/linux/mmc/host.h    | 1 -
>  2 files changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH 2/7] mmc: sdio: Drop mmc_claim|release_host() in mmc_sdio_power_restore()

[Ulf Hansson]

The function mmc_sdio_power_restore() is called either from
mmc_sdio_runtime_resume() or from mmc_sdio_hw_reset(). Both callers either
claims/releases the host or require its callers to do so. Therefore let's
drop the redundant calls to mmc_claim|release_host() in
mmc_sdio_power_restore().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 712a7742765e..b3303b7d9956 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1030,14 +1030,10 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 	 *
 	 */
 
-	mmc_claim_host(host);
-
 	ret = mmc_sdio_reinit_card(host, mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
 
-	mmc_release_host(host);
-
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH 2/7] mmc: sdio: Drop mmc_claim|release_host() in mmc_sdio_power_restore()

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The function mmc_sdio_power_restore() is called either from
> mmc_sdio_runtime_resume() or from mmc_sdio_hw_reset(). Both callers either
> claims/releases the host or require its callers to do so. Therefore let's
> drop the redundant calls to mmc_claim|release_host() in
> mmc_sdio_power_restore().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 4 ----
>  1 file changed, 4 deletions(-)

Without being an expert, I looked through the code and as far as I can
tell this is fine.  Specifically:

* I agree there are two calls and mmc_sdio_runtime_resume() clearly claims.

* The only call to mmc_sdio_hw_reset() looks to be mmc_hw_reset();
looking through calls to mmc_hw_reset() I see the claims.  It's super
obvious in the 3 cases in "drivers/net" and I decided that I didn't
need to look at block.c because that shouldn't be a codepath that
affects SDIO.

Thus:

Reviewed-by: Douglas Anderson <dianders@chromium.org>


I also don't see this patch causing any problems on the rk3288-veyron
boards I tested it on.  Thus:

Tested-by: Douglas Anderson <dianders@chromium.org>

[PATCH 3/7] mmc: sdio: Move comment about re-initialization to mmc_sdio_reinit_card()

[Ulf Hansson]

The comment in mmc_sdio_power_restore() belongs in mmc_sdio_reinit_card(),
which was created during a previous commit that re-factored some code. Fix
this by moving the comment into mmc_sdio_reinit_card().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index b3303b7d9956..29f86c1e9923 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -820,6 +820,23 @@ static int mmc_sdio_reinit_card(struct mmc_host *host, bool powered_resume)
 {
 	int ret;
 
+	/*
+	 * Reset the card by performing the same steps that are taken by
+	 * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe.
+	 *
+	 * sdio_reset() is technically not needed. Having just powered up the
+	 * hardware, it should already be in reset state. However, some
+	 * platforms (such as SD8686 on OLPC) do not instantly cut power,
+	 * meaning that a reset is required when restoring power soon after
+	 * powering off. It is harmless in other cases.
+	 *
+	 * The CMD5 reset (mmc_send_io_op_cond()), according to the SDIO spec,
+	 * is not necessary for non-removable cards. However, it is required
+	 * for OLPC SD8686 (which expects a [CMD5,5,3,7] init sequence), and
+	 * harmless in other situations.
+	 *
+	 */
+
 	sdio_reset(host);
 	mmc_go_idle(host);
 	mmc_send_if_cond(host, host->card->ocr);
@@ -1013,23 +1030,6 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
 {
 	int ret;
 
-	/*
-	 * Reset the card by performing the same steps that are taken by
-	 * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe.
-	 *
-	 * sdio_reset() is technically not needed. Having just powered up the
-	 * hardware, it should already be in reset state. However, some
-	 * platforms (such as SD8686 on OLPC) do not instantly cut power,
-	 * meaning that a reset is required when restoring power soon after
-	 * powering off. It is harmless in other cases.
-	 *
-	 * The CMD5 reset (mmc_send_io_op_cond()), according to the SDIO spec,
-	 * is not necessary for non-removable cards. However, it is required
-	 * for OLPC SD8686 (which expects a [CMD5,5,3,7] init sequence), and
-	 * harmless in other situations.
-	 *
-	 */
-
 	ret = mmc_sdio_reinit_card(host, mmc_card_keep_power(host));
 	if (!ret && host->sdio_irqs)
 		mmc_signal_sdio_irq(host);
-- 
2.17.1

Re: [PATCH 3/7] mmc: sdio: Move comment about re-initialization to mmc_sdio_reinit_card()

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The comment in mmc_sdio_power_restore() belongs in mmc_sdio_reinit_card(),
> which was created during a previous commit that re-factored some code. Fix
> this by moving the comment into mmc_sdio_reinit_card().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH 4/7] mmc: sdio: Drop powered-on re-init at runtime resume and HW reset

[Ulf Hansson]

To use the so called powered-on re-initialization of an SDIO card, the
power to the card must obviously have stayed on. If not, the initialization
will simply fail.

In the runtime suspend case, the card is always powered off. Hence, let's
drop the support for powered-on re-initialization during runtime resume, as
it doesn't make sense.

Moreover, during a HW reset, the point is to cut the power to the card and
then do fresh re-initialization. Therefore drop the support for powered-on
re-initialization during HW reset.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 29f86c1e9923..a9bfcae8db5b 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1028,13 +1028,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
 
 static int mmc_sdio_power_restore(struct mmc_host *host)
 {
-	int ret;
-
-	ret = mmc_sdio_reinit_card(host, mmc_card_keep_power(host));
-	if (!ret && host->sdio_irqs)
-		mmc_signal_sdio_irq(host);
-
-	return ret;
+	return mmc_sdio_reinit_card(host, 0);
 }
 
 static int mmc_sdio_runtime_suspend(struct mmc_host *host)
-- 
2.17.1

Re: [PATCH 4/7] mmc: sdio: Drop powered-on re-init at runtime resume and HW reset

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> To use the so called powered-on re-initialization of an SDIO card, the
> power to the card must obviously have stayed on. If not, the initialization
> will simply fail.
>
> In the runtime suspend case, the card is always powered off. Hence, let's
> drop the support for powered-on re-initialization during runtime resume, as
> it doesn't make sense.
>
> Moreover, during a HW reset, the point is to cut the power to the card and
> then do fresh re-initialization. Therefore drop the support for powered-on
> re-initialization during HW reset.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)

This has been on my list of things to test for a while but I never
quite got to it...

...and then, today, I spent time bisecting why the "reset"
functionality of miwfiex is broken on my 4.19 kernel [1].  AKA, this
is broken:

cd /sys/kernel/debug/mwifiex/mlan0
echo 1 > reset

I finally bisected the problem and tracked it down to commit
ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs
are enabled"), which embarrassingly has my Tested-by on it.  I guess I
never tested the Marvell reset call.  :-/

I dug a little and found that when the Marvell code did its reset we
ended up getting a call to dw_mci_enable_sdio_irq(enb=0) and never saw
a dw_mci_enable_sdio_irq(enb=1) after.  I tracked it down further and
found that specifically it was the call to mmc_signal_sdio_irq() in
mmc_sdio_power_restore() that was making the call.  The call stack
shown for the "enb=0" call:

[<c071a290>] (dw_mci_enable_sdio_irq) from [<c070a960>]
(mmc_sdio_power_restore+0x98/0xc0)
[<c070a960>] (mmc_sdio_power_restore) from [<c070a9b4>]
(mmc_sdio_reset+0x2c/0x30)
[<c070a9b4>] (mmc_signal_sdio_irq) from [<c06ff160>] (mmc_hw_reset+0xbc/0x138)
[<c06ff160>] (mmc_hw_reset) from [<bf1bbad8>]
(mwifiex_sdio_work+0x5d4/0x678 [mwifiex_sdio])
[<bf1bbad8>] (mwifiex_sdio_work [mwifiex_sdio]) from [<c0247cd0>]
(process_one_work+0x290/0x4b4)

I picked your patch here (which gets rid of the call to
mmc_signal_sdio_irq()) and magically the problem went away because
there is no more call to mmc_signal_sdio_irq().

I personally don't have lots of history about the whole
"powered_resume" code path.  I checked and mmc_card_keep_power() was 0
in my test case of getting called from hw_reset, so the rest of this
patch doesn't affect me at all.  This surprised me a little since I
saw "MMC_PM_KEEP_POWER" being set in mwifiex but then I realized that
it was only set for the duration of suspend and then cleared by the
core.  ;-)

I will also say that I don't have any test case or knowledge of how
SDIO runtime suspend/resume is supposed to work since on dw_mmc SDIO
cards are currently not allowed to runtime suspend anyway.  ;-)


So I guess the result of all that long-winded reply is that for on
rk3288-veyron-jerry:

Fixes: ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when
SDIO IRQs are enabled")
Tested-by: Douglas Anderson <dianders@chromium.org>


One last note is that, though Marvell WiFi works after a reset after
this commit, Marvell Bluetooth (on the same SDIO module) doesn't.  I
guess next week it'll be another bisect...

[1] https://crbug.com/981113



-Doug

Re: [PATCH 4/7] mmc: sdio: Drop powered-on re-init at runtime resume and HW reset

[Ulf Hansson]

On Thu, 4 Jul 2019 at 02:02, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > To use the so called powered-on re-initialization of an SDIO card, the
> > power to the card must obviously have stayed on. If not, the initialization
> > will simply fail.
> >
> > In the runtime suspend case, the card is always powered off. Hence, let's
> > drop the support for powered-on re-initialization during runtime resume, as
> > it doesn't make sense.
> >
> > Moreover, during a HW reset, the point is to cut the power to the card and
> > then do fresh re-initialization. Therefore drop the support for powered-on
> > re-initialization during HW reset.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/mmc/core/sdio.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
>
> This has been on my list of things to test for a while but I never
> quite got to it...
>
> ...and then, today, I spent time bisecting why the "reset"
> functionality of miwfiex is broken on my 4.19 kernel [1].  AKA, this
> is broken:
>
> cd /sys/kernel/debug/mwifiex/mlan0
> echo 1 > reset
>
> I finally bisected the problem and tracked it down to commit
> ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs
> are enabled"), which embarrassingly has my Tested-by on it.  I guess I
> never tested the Marvell reset call.  :-/
>
> I dug a little and found that when the Marvell code did its reset we
> ended up getting a call to dw_mci_enable_sdio_irq(enb=0) and never saw
> a dw_mci_enable_sdio_irq(enb=1) after.  I tracked it down further and
> found that specifically it was the call to mmc_signal_sdio_irq() in
> mmc_sdio_power_restore() that was making the call.  The call stack
> shown for the "enb=0" call:
>
> [<c071a290>] (dw_mci_enable_sdio_irq) from [<c070a960>]
> (mmc_sdio_power_restore+0x98/0xc0)
> [<c070a960>] (mmc_sdio_power_restore) from [<c070a9b4>]
> (mmc_sdio_reset+0x2c/0x30)
> [<c070a9b4>] (mmc_signal_sdio_irq) from [<c06ff160>] (mmc_hw_reset+0xbc/0x138)
> [<c06ff160>] (mmc_hw_reset) from [<bf1bbad8>]
> (mwifiex_sdio_work+0x5d4/0x678 [mwifiex_sdio])
> [<bf1bbad8>] (mwifiex_sdio_work [mwifiex_sdio]) from [<c0247cd0>]
> (process_one_work+0x290/0x4b4)
>
> I picked your patch here (which gets rid of the call to
> mmc_signal_sdio_irq()) and magically the problem went away because
> there is no more call to mmc_signal_sdio_irq().
>
> I personally don't have lots of history about the whole
> "powered_resume" code path.  I checked and mmc_card_keep_power() was 0
> in my test case of getting called from hw_reset, so the rest of this
> patch doesn't affect me at all.  This surprised me a little since I
> saw "MMC_PM_KEEP_POWER" being set in mwifiex but then I realized that
> it was only set for the duration of suspend and then cleared by the
> core.  ;-)
>
> I will also say that I don't have any test case or knowledge of how
> SDIO runtime suspend/resume is supposed to work since on dw_mmc SDIO
> cards are currently not allowed to runtime suspend anyway.  ;-)
>
>
> So I guess the result of all that long-winded reply is that for on
> rk3288-veyron-jerry:
>
> Fixes: ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when
> SDIO IRQs are enabled")
> Tested-by: Douglas Anderson <dianders@chromium.org>

Thanks a lot for testing and for your detailed feedback. I have
amended the patch by adding your tags above.

Moreover, we seems to need this for stable as well, but I am leaving
that to be managed as a separate task. We may even consider the hole
series for stable, but that requires more testing first.

>
>
> One last note is that, though Marvell WiFi works after a reset after
> this commit, Marvell Bluetooth (on the same SDIO module) doesn't.  I
> guess next week it'll be another bisect...

Is the Bluetooth connected to the same SDIO interface, thus the
Bluetooth driver is an SDIO func driver?

>
> [1] https://crbug.com/981113
>
>
>
> -Doug

Kind regards
Uffe

Re: [PATCH 4/7] mmc: sdio: Drop powered-on re-init at runtime resume and HW reset

[Doug Anderson]

Hi,

On Mon, Jul 8, 2019 at 3:54 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 4 Jul 2019 at 02:02, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > To use the so called powered-on re-initialization of an SDIO card, the
> > > power to the card must obviously have stayed on. If not, the initialization
> > > will simply fail.
> > >
> > > In the runtime suspend case, the card is always powered off. Hence, let's
> > > drop the support for powered-on re-initialization during runtime resume, as
> > > it doesn't make sense.
> > >
> > > Moreover, during a HW reset, the point is to cut the power to the card and
> > > then do fresh re-initialization. Therefore drop the support for powered-on
> > > re-initialization during HW reset.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/mmc/core/sdio.c | 8 +-------
> > >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > This has been on my list of things to test for a while but I never
> > quite got to it...
> >
> > ...and then, today, I spent time bisecting why the "reset"
> > functionality of miwfiex is broken on my 4.19 kernel [1].  AKA, this
> > is broken:
> >
> > cd /sys/kernel/debug/mwifiex/mlan0
> > echo 1 > reset
> >
> > I finally bisected the problem and tracked it down to commit
> > ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs
> > are enabled"), which embarrassingly has my Tested-by on it.  I guess I
> > never tested the Marvell reset call.  :-/
> >
> > I dug a little and found that when the Marvell code did its reset we
> > ended up getting a call to dw_mci_enable_sdio_irq(enb=0) and never saw
> > a dw_mci_enable_sdio_irq(enb=1) after.  I tracked it down further and
> > found that specifically it was the call to mmc_signal_sdio_irq() in
> > mmc_sdio_power_restore() that was making the call.  The call stack
> > shown for the "enb=0" call:
> >
> > [<c071a290>] (dw_mci_enable_sdio_irq) from [<c070a960>]
> > (mmc_sdio_power_restore+0x98/0xc0)
> > [<c070a960>] (mmc_sdio_power_restore) from [<c070a9b4>]
> > (mmc_sdio_reset+0x2c/0x30)
> > [<c070a9b4>] (mmc_signal_sdio_irq) from [<c06ff160>] (mmc_hw_reset+0xbc/0x138)
> > [<c06ff160>] (mmc_hw_reset) from [<bf1bbad8>]
> > (mwifiex_sdio_work+0x5d4/0x678 [mwifiex_sdio])
> > [<bf1bbad8>] (mwifiex_sdio_work [mwifiex_sdio]) from [<c0247cd0>]
> > (process_one_work+0x290/0x4b4)
> >
> > I picked your patch here (which gets rid of the call to
> > mmc_signal_sdio_irq()) and magically the problem went away because
> > there is no more call to mmc_signal_sdio_irq().
> >
> > I personally don't have lots of history about the whole
> > "powered_resume" code path.  I checked and mmc_card_keep_power() was 0
> > in my test case of getting called from hw_reset, so the rest of this
> > patch doesn't affect me at all.  This surprised me a little since I
> > saw "MMC_PM_KEEP_POWER" being set in mwifiex but then I realized that
> > it was only set for the duration of suspend and then cleared by the
> > core.  ;-)
> >
> > I will also say that I don't have any test case or knowledge of how
> > SDIO runtime suspend/resume is supposed to work since on dw_mmc SDIO
> > cards are currently not allowed to runtime suspend anyway.  ;-)
> >
> >
> > So I guess the result of all that long-winded reply is that for on
> > rk3288-veyron-jerry:
> >
> > Fixes: ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when
> > SDIO IRQs are enabled")
> > Tested-by: Douglas Anderson <dianders@chromium.org>
>
> Thanks a lot for testing and for your detailed feedback. I have
> amended the patch by adding your tags above.

Sure!  I'm going to try to do some detailed testing on the next patch
too to confirm it's OK, but I have a few other tasks to get to first.
;-)


> Moreover, we seems to need this for stable as well, but I am leaving
> that to be managed as a separate task. We may even consider the hole
> series for stable, but that requires more testing first.

Sure, makes sense.  We'll pick it to the Chrome OS 4.19 kernel
directly but it's usually nice to get fixes like this into stable so
everyone can benefit.


> > One last note is that, though Marvell WiFi works after a reset after
> > this commit, Marvell Bluetooth (on the same SDIO module) doesn't.  I
> > guess next week it'll be another bisect...
>
> Is the Bluetooth connected to the same SDIO interface, thus the
> Bluetooth driver is an SDIO func driver?

Yes, it's a SDIO func driver connected to the same interface.  So far
I've managed to confirm the problem on:

v4.4 (plus 76ae3e26ea43 mwifiex: add debugfs file for testing reset of card)
v4.9
next-20190708

...so it seems like it's not a "regression", it's just never worked.
:-P  I guess I'll have to see if I can figure out what's different in
our chromeos-3.14 kernel.  Ah, I see.  In 3.14 we had this solution:

pr_err("Resetting card...\n");
mmc_remove_host(reset_host);
/* 200ms delay is based on experiment with sdhci controller */
mdelay(200);
reset_host->rescan_entered = 0;
mmc_add_host(reset_host);

...I think that didn't fly upstream.  ...but I can confirm that this works fine:

cd /sys/bus/platform/drivers/dwmmc_rockchip
echo ff0d0000.dwmmc > unbind
sleep .5
echo ff0d0000.dwmmc > bind

...so I guess this boils down to: how does the mwifiex reset code not
behave like a full removal and re-insertion of the card?  Oh, but
maybe that's obvious.  We're doing all the reset / re-init from the
WiFi side of things (mwifiex_sdio_card_reset_work) but I don't think
anything is communicated to the Bluetooth side of things.  Presumably
this is just totally broken for everyone?  ...or am I confused?


-Doug


-Doug

Re: [PATCH 4/7] mmc: sdio: Drop powered-on re-init at runtime resume and HW reset

[Ulf Hansson]

On Mon, 8 Jul 2019 at 23:12, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Jul 8, 2019 at 3:54 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 4 Jul 2019 at 02:02, Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > To use the so called powered-on re-initialization of an SDIO card, the
> > > > power to the card must obviously have stayed on. If not, the initialization
> > > > will simply fail.
> > > >
> > > > In the runtime suspend case, the card is always powered off. Hence, let's
> > > > drop the support for powered-on re-initialization during runtime resume, as
> > > > it doesn't make sense.
> > > >
> > > > Moreover, during a HW reset, the point is to cut the power to the card and
> > > > then do fresh re-initialization. Therefore drop the support for powered-on
> > > > re-initialization during HW reset.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/mmc/core/sdio.c | 8 +-------
> > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > >
> > > This has been on my list of things to test for a while but I never
> > > quite got to it...
> > >
> > > ...and then, today, I spent time bisecting why the "reset"
> > > functionality of miwfiex is broken on my 4.19 kernel [1].  AKA, this
> > > is broken:
> > >
> > > cd /sys/kernel/debug/mwifiex/mlan0
> > > echo 1 > reset
> > >
> > > I finally bisected the problem and tracked it down to commit
> > > ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs
> > > are enabled"), which embarrassingly has my Tested-by on it.  I guess I
> > > never tested the Marvell reset call.  :-/
> > >
> > > I dug a little and found that when the Marvell code did its reset we
> > > ended up getting a call to dw_mci_enable_sdio_irq(enb=0) and never saw
> > > a dw_mci_enable_sdio_irq(enb=1) after.  I tracked it down further and
> > > found that specifically it was the call to mmc_signal_sdio_irq() in
> > > mmc_sdio_power_restore() that was making the call.  The call stack
> > > shown for the "enb=0" call:
> > >
> > > [<c071a290>] (dw_mci_enable_sdio_irq) from [<c070a960>]
> > > (mmc_sdio_power_restore+0x98/0xc0)
> > > [<c070a960>] (mmc_sdio_power_restore) from [<c070a9b4>]
> > > (mmc_sdio_reset+0x2c/0x30)
> > > [<c070a9b4>] (mmc_signal_sdio_irq) from [<c06ff160>] (mmc_hw_reset+0xbc/0x138)
> > > [<c06ff160>] (mmc_hw_reset) from [<bf1bbad8>]
> > > (mwifiex_sdio_work+0x5d4/0x678 [mwifiex_sdio])
> > > [<bf1bbad8>] (mwifiex_sdio_work [mwifiex_sdio]) from [<c0247cd0>]
> > > (process_one_work+0x290/0x4b4)
> > >
> > > I picked your patch here (which gets rid of the call to
> > > mmc_signal_sdio_irq()) and magically the problem went away because
> > > there is no more call to mmc_signal_sdio_irq().
> > >
> > > I personally don't have lots of history about the whole
> > > "powered_resume" code path.  I checked and mmc_card_keep_power() was 0
> > > in my test case of getting called from hw_reset, so the rest of this
> > > patch doesn't affect me at all.  This surprised me a little since I
> > > saw "MMC_PM_KEEP_POWER" being set in mwifiex but then I realized that
> > > it was only set for the duration of suspend and then cleared by the
> > > core.  ;-)
> > >
> > > I will also say that I don't have any test case or knowledge of how
> > > SDIO runtime suspend/resume is supposed to work since on dw_mmc SDIO
> > > cards are currently not allowed to runtime suspend anyway.  ;-)
> > >
> > >
> > > So I guess the result of all that long-winded reply is that for on
> > > rk3288-veyron-jerry:
> > >
> > > Fixes: ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when
> > > SDIO IRQs are enabled")
> > > Tested-by: Douglas Anderson <dianders@chromium.org>
> >
> > Thanks a lot for testing and for your detailed feedback. I have
> > amended the patch by adding your tags above.
>
> Sure!  I'm going to try to do some detailed testing on the next patch
> too to confirm it's OK, but I have a few other tasks to get to first.
> ;-)
>
>
> > Moreover, we seems to need this for stable as well, but I am leaving
> > that to be managed as a separate task. We may even consider the hole
> > series for stable, but that requires more testing first.
>
> Sure, makes sense.  We'll pick it to the Chrome OS 4.19 kernel
> directly but it's usually nice to get fixes like this into stable so
> everyone can benefit.
>
>
> > > One last note is that, though Marvell WiFi works after a reset after
> > > this commit, Marvell Bluetooth (on the same SDIO module) doesn't.  I
> > > guess next week it'll be another bisect...
> >
> > Is the Bluetooth connected to the same SDIO interface, thus the
> > Bluetooth driver is an SDIO func driver?
>
> Yes, it's a SDIO func driver connected to the same interface.  So far
> I've managed to confirm the problem on:
>
> v4.4 (plus 76ae3e26ea43 mwifiex: add debugfs file for testing reset of card)
> v4.9
> next-20190708
>
> ...so it seems like it's not a "regression", it's just never worked.
> :-P  I guess I'll have to see if I can figure out what's different in
> our chromeos-3.14 kernel.  Ah, I see.  In 3.14 we had this solution:
>
> pr_err("Resetting card...\n");
> mmc_remove_host(reset_host);
> /* 200ms delay is based on experiment with sdhci controller */
> mdelay(200);
> reset_host->rescan_entered = 0;
> mmc_add_host(reset_host);
>
> ...I think that didn't fly upstream.  ...but I can confirm that this works fine:
>
> cd /sys/bus/platform/drivers/dwmmc_rockchip
> echo ff0d0000.dwmmc > unbind
> sleep .5
> echo ff0d0000.dwmmc > bind
>
> ...so I guess this boils down to: how does the mwifiex reset code not
> behave like a full removal and re-insertion of the card?  Oh, but
> maybe that's obvious.  We're doing all the reset / re-init from the
> WiFi side of things (mwifiex_sdio_card_reset_work) but I don't think
> anything is communicated to the Bluetooth side of things.  Presumably
> this is just totally broken for everyone?  ...or am I confused?

Nope, that is most likely what is happening.

I am not sure what is the best method to deal with this. Perhaps we
should invent some callback the SDIO core code can call, for each
active SDIO func on the particular SDIO card that becomes reset.

Or is there a better way you think?

Kind regards
Uffe

Re: [PATCH 4/7] mmc: sdio: Drop powered-on re-init at runtime resume and HW reset

[Doug Anderson]

Hi,

On Tue, Jul 9, 2019 at 5:02 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 8 Jul 2019 at 23:12, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 8, 2019 at 3:54 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Thu, 4 Jul 2019 at 02:02, Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > To use the so called powered-on re-initialization of an SDIO card, the
> > > > > power to the card must obviously have stayed on. If not, the initialization
> > > > > will simply fail.
> > > > >
> > > > > In the runtime suspend case, the card is always powered off. Hence, let's
> > > > > drop the support for powered-on re-initialization during runtime resume, as
> > > > > it doesn't make sense.
> > > > >
> > > > > Moreover, during a HW reset, the point is to cut the power to the card and
> > > > > then do fresh re-initialization. Therefore drop the support for powered-on
> > > > > re-initialization during HW reset.
> > > > >
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > >  drivers/mmc/core/sdio.c | 8 +-------
> > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > >
> > > > This has been on my list of things to test for a while but I never
> > > > quite got to it...
> > > >
> > > > ...and then, today, I spent time bisecting why the "reset"
> > > > functionality of miwfiex is broken on my 4.19 kernel [1].  AKA, this
> > > > is broken:
> > > >
> > > > cd /sys/kernel/debug/mwifiex/mlan0
> > > > echo 1 > reset
> > > >
> > > > I finally bisected the problem and tracked it down to commit
> > > > ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs
> > > > are enabled"), which embarrassingly has my Tested-by on it.  I guess I
> > > > never tested the Marvell reset call.  :-/
> > > >
> > > > I dug a little and found that when the Marvell code did its reset we
> > > > ended up getting a call to dw_mci_enable_sdio_irq(enb=0) and never saw
> > > > a dw_mci_enable_sdio_irq(enb=1) after.  I tracked it down further and
> > > > found that specifically it was the call to mmc_signal_sdio_irq() in
> > > > mmc_sdio_power_restore() that was making the call.  The call stack
> > > > shown for the "enb=0" call:
> > > >
> > > > [<c071a290>] (dw_mci_enable_sdio_irq) from [<c070a960>]
> > > > (mmc_sdio_power_restore+0x98/0xc0)
> > > > [<c070a960>] (mmc_sdio_power_restore) from [<c070a9b4>]
> > > > (mmc_sdio_reset+0x2c/0x30)
> > > > [<c070a9b4>] (mmc_signal_sdio_irq) from [<c06ff160>] (mmc_hw_reset+0xbc/0x138)
> > > > [<c06ff160>] (mmc_hw_reset) from [<bf1bbad8>]
> > > > (mwifiex_sdio_work+0x5d4/0x678 [mwifiex_sdio])
> > > > [<bf1bbad8>] (mwifiex_sdio_work [mwifiex_sdio]) from [<c0247cd0>]
> > > > (process_one_work+0x290/0x4b4)
> > > >
> > > > I picked your patch here (which gets rid of the call to
> > > > mmc_signal_sdio_irq()) and magically the problem went away because
> > > > there is no more call to mmc_signal_sdio_irq().
> > > >
> > > > I personally don't have lots of history about the whole
> > > > "powered_resume" code path.  I checked and mmc_card_keep_power() was 0
> > > > in my test case of getting called from hw_reset, so the rest of this
> > > > patch doesn't affect me at all.  This surprised me a little since I
> > > > saw "MMC_PM_KEEP_POWER" being set in mwifiex but then I realized that
> > > > it was only set for the duration of suspend and then cleared by the
> > > > core.  ;-)
> > > >
> > > > I will also say that I don't have any test case or knowledge of how
> > > > SDIO runtime suspend/resume is supposed to work since on dw_mmc SDIO
> > > > cards are currently not allowed to runtime suspend anyway.  ;-)
> > > >
> > > >
> > > > So I guess the result of all that long-winded reply is that for on
> > > > rk3288-veyron-jerry:
> > > >
> > > > Fixes: ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when
> > > > SDIO IRQs are enabled")
> > > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > >
> > > Thanks a lot for testing and for your detailed feedback. I have
> > > amended the patch by adding your tags above.
> >
> > Sure!  I'm going to try to do some detailed testing on the next patch
> > too to confirm it's OK, but I have a few other tasks to get to first.
> > ;-)
> >
> >
> > > Moreover, we seems to need this for stable as well, but I am leaving
> > > that to be managed as a separate task. We may even consider the hole
> > > series for stable, but that requires more testing first.
> >
> > Sure, makes sense.  We'll pick it to the Chrome OS 4.19 kernel
> > directly but it's usually nice to get fixes like this into stable so
> > everyone can benefit.
> >
> >
> > > > One last note is that, though Marvell WiFi works after a reset after
> > > > this commit, Marvell Bluetooth (on the same SDIO module) doesn't.  I
> > > > guess next week it'll be another bisect...
> > >
> > > Is the Bluetooth connected to the same SDIO interface, thus the
> > > Bluetooth driver is an SDIO func driver?
> >
> > Yes, it's a SDIO func driver connected to the same interface.  So far
> > I've managed to confirm the problem on:
> >
> > v4.4 (plus 76ae3e26ea43 mwifiex: add debugfs file for testing reset of card)
> > v4.9
> > next-20190708
> >
> > ...so it seems like it's not a "regression", it's just never worked.
> > :-P  I guess I'll have to see if I can figure out what's different in
> > our chromeos-3.14 kernel.  Ah, I see.  In 3.14 we had this solution:
> >
> > pr_err("Resetting card...\n");
> > mmc_remove_host(reset_host);
> > /* 200ms delay is based on experiment with sdhci controller */
> > mdelay(200);
> > reset_host->rescan_entered = 0;
> > mmc_add_host(reset_host);
> >
> > ...I think that didn't fly upstream.  ...but I can confirm that this works fine:
> >
> > cd /sys/bus/platform/drivers/dwmmc_rockchip
> > echo ff0d0000.dwmmc > unbind
> > sleep .5
> > echo ff0d0000.dwmmc > bind
> >
> > ...so I guess this boils down to: how does the mwifiex reset code not
> > behave like a full removal and re-insertion of the card?  Oh, but
> > maybe that's obvious.  We're doing all the reset / re-init from the
> > WiFi side of things (mwifiex_sdio_card_reset_work) but I don't think
> > anything is communicated to the Bluetooth side of things.  Presumably
> > this is just totally broken for everyone?  ...or am I confused?
>
> Nope, that is most likely what is happening.
>
> I am not sure what is the best method to deal with this. Perhaps we
> should invent some callback the SDIO core code can call, for each
> active SDIO func on the particular SDIO card that becomes reset.
>
> Or is there a better way you think?

I didn't get a chance to fully dig today, but I keep thinking that the
cleanest way would be if I could somehow tell the MMC core to pretend
to unplug the card and then re-plug it in.  Then we could go through
all the standard code paths.  I remember the last time I looked for a
nice way to do that I couldn't find one.

...barring that I wonder if it's enough to just remove and re-add all
the active SDIO funcs?

-Doug

[PATCH 5/7] mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume

[Ulf Hansson]

It looks like the original idea behind always doing a re-initialization of
a removable SDIO card during system resume in mmc_sdio_resume(), is to try
to play safe to detect whether the card has been removed.

However, this seems like a really a bad idea as it will most likely screw
things up, especially when the card is expected to remain powered on during
system suspend by the SDIO func driver.

Let's fix this, simply by trusting that the detect work checks if the card
is alive and inserted, which is being scheduled at the PM_POST_SUSPEND
notification anyway.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index a9bfcae8db5b..945416c53b56 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -982,7 +982,11 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	/* Basic card reinitialization. */
 	mmc_claim_host(host);
 
-	/* Restore power if needed */
+	/*
+	 * Restore power and reinitialize the card when needed. Note that a
+	 * removable card is checked from a detect work later on in the resume
+	 * process.
+	 */
 	if (!mmc_card_keep_power(host)) {
 		mmc_power_up(host, host->card->ocr);
 		/*
@@ -996,12 +1000,8 @@ static int mmc_sdio_resume(struct mmc_host *host)
 			pm_runtime_set_active(&host->card->dev);
 			pm_runtime_enable(&host->card->dev);
 		}
-	}
-
-	/* No need to reinitialize powered-resumed nonremovable cards */
-	if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
-		err = mmc_sdio_reinit_card(host, mmc_card_keep_power(host));
-	} else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
+		err = mmc_sdio_reinit_card(host, 0);
+	} else if (mmc_card_wake_sdio_irq(host)) {
 		/* We may have switched to 1-bit mode during suspend */
 		err = sdio_enable_4bit_bus(host->card);
 	}
-- 
2.17.1

Re: [PATCH 5/7] mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> It looks like the original idea behind always doing a re-initialization of
> a removable SDIO card during system resume in mmc_sdio_resume(), is to try
> to play safe to detect whether the card has been removed.
>
> However, this seems like a really a bad idea as it will most likely screw
> things up, especially when the card is expected to remain powered on during
> system suspend by the SDIO func driver.
>
> Let's fix this, simply by trusting that the detect work checks if the card
> is alive and inserted, which is being scheduled at the PM_POST_SUSPEND
> notification anyway.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

I'm not sure if it's even worth mentioning since the device I tested
was rk3288-veyron (both WiFi variants) and we have a non-removable,
powered-in-suspend, wakeup-disabled card.  That means it isn't
affected at all by your change.  ...but I suppose I can at least
confirm that your change didn't break me if that's worth anything.
:-P

Tested-by: Douglas Anderson <dianders@chromium.org>

I would also say that, though I don't have any history here, your
patch seems reasonable to me.  So you can add a:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH 6/7] mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()

[Ulf Hansson]

The "powered_resume" in-parameter to mmc_sdio_reinit_card() has now become
redundant as all callers set it to 0. Therefore let's just drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 945416c53b56..0bf603670f61 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -816,7 +816,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	return err;
 }
 
-static int mmc_sdio_reinit_card(struct mmc_host *host, bool powered_resume)
+static int mmc_sdio_reinit_card(struct mmc_host *host)
 {
 	int ret;
 
@@ -845,8 +845,7 @@ static int mmc_sdio_reinit_card(struct mmc_host *host, bool powered_resume)
 	if (ret)
 		return ret;
 
-	return mmc_sdio_init_card(host, host->card->ocr, host->card,
-				  powered_resume);
+	return mmc_sdio_init_card(host, host->card->ocr, host->card, 0);
 }
 
 /*
@@ -1000,7 +999,7 @@ static int mmc_sdio_resume(struct mmc_host *host)
 			pm_runtime_set_active(&host->card->dev);
 			pm_runtime_enable(&host->card->dev);
 		}
-		err = mmc_sdio_reinit_card(host, 0);
+		err = mmc_sdio_reinit_card(host);
 	} else if (mmc_card_wake_sdio_irq(host)) {
 		/* We may have switched to 1-bit mode during suspend */
 		err = sdio_enable_4bit_bus(host->card);
@@ -1026,11 +1025,6 @@ static int mmc_sdio_resume(struct mmc_host *host)
 	return err;
 }
 
-static int mmc_sdio_power_restore(struct mmc_host *host)
-{
-	return mmc_sdio_reinit_card(host, 0);
-}
-
 static int mmc_sdio_runtime_suspend(struct mmc_host *host)
 {
 	/* No references to the card, cut the power to it. */
@@ -1048,7 +1042,7 @@ static int mmc_sdio_runtime_resume(struct mmc_host *host)
 	/* Restore power and re-initialize. */
 	mmc_claim_host(host);
 	mmc_power_up(host, host->card->ocr);
-	ret = mmc_sdio_power_restore(host);
+	ret = mmc_sdio_reinit_card(host);
 	mmc_release_host(host);
 
 	return ret;
@@ -1057,7 +1051,7 @@ static int mmc_sdio_runtime_resume(struct mmc_host *host)
 static int mmc_sdio_hw_reset(struct mmc_host *host)
 {
 	mmc_power_cycle(host, host->card->ocr);
-	return mmc_sdio_power_restore(host);
+	return mmc_sdio_reinit_card(host);
 }
 
 static int mmc_sdio_sw_reset(struct mmc_host *host)
@@ -1069,7 +1063,7 @@ static int mmc_sdio_sw_reset(struct mmc_host *host)
 	mmc_set_initial_state(host);
 	mmc_set_initial_signal_voltage(host);
 
-	return mmc_sdio_reinit_card(host, 0);
+	return mmc_sdio_reinit_card(host);
 }
 
 static const struct mmc_bus_ops mmc_sdio_ops = {
-- 
2.17.1

Re: [PATCH 6/7] mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The "powered_resume" in-parameter to mmc_sdio_reinit_card() has now become
> redundant as all callers set it to 0. Therefore let's just drop it.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

[PATCH 7/7] mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()

[Ulf Hansson]

The "powered_resume" in-parameter to mmc_sdio_init_card() has now become
redundant as all callers set it to 0. Therefore let's just drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 0bf603670f61..8dd8fc32ecca 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -559,7 +559,7 @@ static void mmc_sdio_resend_if_cond(struct mmc_host *host,
  * we're trying to reinitialise.
  */
 static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
-			      struct mmc_card *oldcard, int powered_resume)
+			      struct mmc_card *oldcard)
 {
 	struct mmc_card *card;
 	int err;
@@ -582,11 +582,9 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	/*
 	 * Inform the card of the voltage
 	 */
-	if (!powered_resume) {
-		err = mmc_send_io_op_cond(host, ocr, &rocr);
-		if (err)
-			goto err;
-	}
+	err = mmc_send_io_op_cond(host, ocr, &rocr);
+	if (err)
+		goto err;
 
 	/*
 	 * For SPI, enable CRC as appropriate.
@@ -645,7 +643,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	 * try to init uhs card. sdio_read_cccr will take over this task
 	 * to make sure which speed mode should work.
 	 */
-	if (!powered_resume && (rocr & ocr & R4_18V_PRESENT)) {
+	if (rocr & ocr & R4_18V_PRESENT) {
 		err = mmc_set_uhs_voltage(host, ocr_card);
 		if (err == -EAGAIN) {
 			mmc_sdio_resend_if_cond(host, card);
@@ -659,7 +657,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	/*
 	 * For native busses:  set card RCA and quit open drain mode.
 	 */
-	if (!powered_resume && !mmc_host_is_spi(host)) {
+	if (!mmc_host_is_spi(host)) {
 		err = mmc_send_relative_addr(host, &card->rca);
 		if (err)
 			goto remove;
@@ -687,7 +685,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	/*
 	 * Select card, as all following commands rely on that.
 	 */
-	if (!powered_resume && !mmc_host_is_spi(host)) {
+	if (!mmc_host_is_spi(host)) {
 		err = mmc_select_card(card);
 		if (err)
 			goto remove;
@@ -845,7 +843,7 @@ static int mmc_sdio_reinit_card(struct mmc_host *host)
 	if (ret)
 		return ret;
 
-	return mmc_sdio_init_card(host, host->card->ocr, host->card, 0);
+	return mmc_sdio_init_card(host, host->card->ocr, host->card);
 }
 
 /*
@@ -1113,7 +1111,7 @@ int mmc_attach_sdio(struct mmc_host *host)
 	/*
 	 * Detect and init the card.
 	 */
-	err = mmc_sdio_init_card(host, rocr, NULL, 0);
+	err = mmc_sdio_init_card(host, rocr, NULL);
 	if (err)
 		goto err;
 
-- 
2.17.1

Re: [PATCH 7/7] mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The "powered_resume" in-parameter to mmc_sdio_init_card() has now become
> redundant as all callers set it to 0. Therefore let's just drop it.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 0/7] mmc: sdio: Various fixes/improvements for SDIO PM

[Ulf Hansson]

On Tue, 18 Jun 2019 at 17:34, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The power management support implemented via system suspend/resume and runtime
> suspend/resume for SDIO cards is rather messy, but also fragile.
>
> This series makes some improvement to this code. In particular the so called
> powered-on re-initialization of SDIO card is quite questionable, I suspect
> it may never really worked well. Therefore this series removes this code, which
> helps to prepare for additional improvements on top in a later series.
>
> So far the series has only been compile tested, so any help in testing on HW for
> regressions is greatly appreciated.
>
> Kind regards
> Uffe
>
> Ulf Hansson (7):
>   mmc: sdio: Turn sdio_run_irqs() into static
>   mmc: sdio: Drop mmc_claim|release_host() in mmc_sdio_power_restore()
>   mmc: sdio: Move comment about re-initialization to
>     mmc_sdio_reinit_card()
>   mmc: sdio: Drop powered-on re-init at runtime resume and HW reset
>   mmc: sdio: Don't re-initialize powered-on removable SDIO cards at
>     resume
>   mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()
>   mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()
>
>  drivers/mmc/core/sdio.c     | 92 +++++++++++++++----------------------
>  drivers/mmc/core/sdio_irq.c |  3 +-
>  include/linux/mmc/host.h    |  1 -
>  3 files changed, 38 insertions(+), 58 deletions(-)
>
> --
> 2.17.1
>

I decided to queue this up, to see what tests from linux-next and
kernelCI reports.

Still, that doesn't mean I am appreciating test done on HW. I can also
apply tested-by tags by amending patches after this point.

Kind regards
Uffe