[PATCH] ath10k: remove mmc_hw_reset while hif power down

[PATCH] ath10k: remove mmc_hw_reset while hif power down

[Wen Gong]

For sdio 3.0 chip, the clock will drop from 200M Hz to 50M Hz after load
ath10k driver, it is because mmc_hw_reset will reset the sdio's power,
then mmc will consider it as sdio 2.0 and drop the clock.

Remove mmc_hw_reset will avoid the drop of clock.

Tested with QCA6174 SDIO with firmware
WLAN.RMH.4.4.1-00007-QCARMSWP-1.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/sdio.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index fae56c6..f1d2af8 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -1433,10 +1433,6 @@ static void ath10k_sdio_hif_power_down(struct ath10k *ar)
 		return;
 	}
 
-	ret = mmc_hw_reset(ar_sdio->func->card->host);
-	if (ret)
-		ath10k_warn(ar, "unable to reset sdio: %d\n", ret);
-
 	sdio_release_host(ar_sdio->func);
 
 	ar_sdio->is_disabled = true;
-- 
1.9.1

Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down

[Grant Grundler]

[repeating comments I made in the gerrit review for Chrome OS :
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1585667
]

On Sat, Apr 27, 2019 at 7:17 PM Wen Gong <wgong@codeaurora.org> wrote:
>
> For sdio 3.0 chip, the clock will drop from 200M Hz to 50M Hz after load
> ath10k driver, it is because mmc_hw_reset will reset the sdio's power,
> then mmc will consider it as sdio 2.0 and drop the clock.

Wen,
5468e784c0600551ca03263f5255a375c05f88e7 commit message gives reasons
for adding the mmc_hw_reset() call. The commit message for removing
gives different reason for removal. Both are good but second one is
incomplete.

The commit message for removal should ALSO explain why adding this
call wasn't necessary in the first place OR move the call to a
different code path.

> Remove mmc_hw_reset will avoid the drop of clock.

This commit message makes it clear the original patch introduced a new
problem. But the original patch fixed a different problem and that
this proposed change seems likely to re-introduce and the commit
message should explain why that isn't true (or how the original was
fixed differently)

cheers,
grant

>
> Tested with QCA6174 SDIO with firmware
> WLAN.RMH.4.4.1-00007-QCARMSWP-1.
>
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath10k/sdio.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index fae56c6..f1d2af8 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -1433,10 +1433,6 @@ static void ath10k_sdio_hif_power_down(struct ath10k *ar)
>                 return;
>         }
>
> -       ret = mmc_hw_reset(ar_sdio->func->card->host);
> -       if (ret)
> -               ath10k_warn(ar, "unable to reset sdio: %d\n", ret);
> -
>         sdio_release_host(ar_sdio->func);
>
>         ar_sdio->is_disabled = true;
> --
> 1.9.1
>

RE: [PATCH] ath10k: remove mmc_hw_reset while hif power down

[Wen Gong]

> -----Original Message-----
> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Grant
> Grundler
> Sent: Saturday, May 4, 2019 2:01 AM
> To: Wen Gong <wgong@codeaurora.org>
> Cc: linux-wireless@vger.kernel.org; ath10k@lists.infradead.org
> Subject: [EXT] Re: [PATCH] ath10k: remove mmc_hw_reset while hif power
> down
> 
> [repeating comments I made in the gerrit review for Chrome OS :
> https://chromium-
> review.googlesource.com/c/chromiumos/third_party/kernel/+/1585667
> ]
> 
> On Sat, Apr 27, 2019 at 7:17 PM Wen Gong <wgong@codeaurora.org> wrote:
> >
> > For sdio 3.0 chip, the clock will drop from 200M Hz to 50M Hz after load
> > ath10k driver, it is because mmc_hw_reset will reset the sdio's power,
> > then mmc will consider it as sdio 2.0 and drop the clock.
> 
> Wen,
> 5468e784c0600551ca03263f5255a375c05f88e7 commit message gives
> reasons
> for adding the mmc_hw_reset() call. The commit message for removing
> gives different reason for removal. Both are good but second one is
> incomplete.
> 
> The commit message for removal should ALSO explain why adding this
> call wasn't necessary in the first place OR move the call to a
> different code path.
> 
> > Remove mmc_hw_reset will avoid the drop of clock.
> 
> This commit message makes it clear the original patch introduced a new
> problem. But the original patch fixed a different problem and that
> this proposed change seems likely to re-introduce and the commit
> message should explain why that isn't true (or how the original was
> fixed differently)
Hi Grant,
The mmc_hw_reset's effect depends on the hardware layout/configure
software's behavior, recently it will effect the clock of sdio for the platform
I used. And it will still work well without mmc_hw_reset for the platform I
Used currently. 
If sdio cannot work on other platform, I think it can add flag in 
ath10k_hw_params_list for the platform to call the mmc_hw_reset depends
on the flag.
> 
> cheers,
> grant

Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down

[Kalle Valo]

+ Ulf to give comments from SDIO point of view

Wen Gong <wgong@qti.qualcomm.com> writes:

>> -----Original Message-----
>> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Grant
>> Grundler
>> Sent: Saturday, May 4, 2019 2:01 AM
>> To: Wen Gong <wgong@codeaurora.org>
>> Cc: linux-wireless@vger.kernel.org; ath10k@lists.infradead.org
>> Subject: [EXT] Re: [PATCH] ath10k: remove mmc_hw_reset while hif power
>> down
>> 
>> [repeating comments I made in the gerrit review for Chrome OS :
>> https://chromium-
>> review.googlesource.com/c/chromiumos/third_party/kernel/+/1585667
>> ]
>> 
>> On Sat, Apr 27, 2019 at 7:17 PM Wen Gong <wgong@codeaurora.org> wrote:
>> >
>> > For sdio 3.0 chip, the clock will drop from 200M Hz to 50M Hz after load
>> > ath10k driver, it is because mmc_hw_reset will reset the sdio's power,
>> > then mmc will consider it as sdio 2.0 and drop the clock.
>> 
>> Wen,
>> 5468e784c0600551ca03263f5255a375c05f88e7 commit message gives
>> reasons
>> for adding the mmc_hw_reset() call. The commit message for removing
>> gives different reason for removal. Both are good but second one is
>> incomplete.
>> 
>> The commit message for removal should ALSO explain why adding this
>> call wasn't necessary in the first place OR move the call to a
>> different code path.
>> 
>> > Remove mmc_hw_reset will avoid the drop of clock.
>> 
>> This commit message makes it clear the original patch introduced a new
>> problem. But the original patch fixed a different problem and that
>> this proposed change seems likely to re-introduce and the commit
>> message should explain why that isn't true (or how the original was
>> fixed differently)
>
> The mmc_hw_reset's effect depends on the hardware layout/configure
> software's behavior, recently it will effect the clock of sdio for the
> platform I used. And it will still work well without mmc_hw_reset for
> the platform I Used currently. If sdio cannot work on other platform,
> I think it can add flag in ath10k_hw_params_list for the platform to
> call the mmc_hw_reset depends on the flag.

I don't see how you can use ath10k_hw_params_list to separate SDIO
controller functionality, I assume that's the real reason for difference
of functionality? Maybe this is a bug on the SDIO controller?

Ulf, what do you think? Any suggestions? Full discussion here:

https://patchwork.kernel.org/patch/10920563/

-- 
Kalle Valo

Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down

[Ulf Hansson]

On Tue, 7 May 2019 at 11:35, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> + Ulf to give comments from SDIO point of view

Apologize for the delay, it's been a busy period.

>
> Wen Gong <wgong@qti.qualcomm.com> writes:
>
> >> -----Original Message-----
> >> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Grant
> >> Grundler
> >> Sent: Saturday, May 4, 2019 2:01 AM
> >> To: Wen Gong <wgong@codeaurora.org>
> >> Cc: linux-wireless@vger.kernel.org; ath10k@lists.infradead.org
> >> Subject: [EXT] Re: [PATCH] ath10k: remove mmc_hw_reset while hif power
> >> down
> >>
> >> [repeating comments I made in the gerrit review for Chrome OS :
> >> https://chromium-
> >> review.googlesource.com/c/chromiumos/third_party/kernel/+/1585667
> >> ]
> >>
> >> On Sat, Apr 27, 2019 at 7:17 PM Wen Gong <wgong@codeaurora.org> wrote:
> >> >
> >> > For sdio 3.0 chip, the clock will drop from 200M Hz to 50M Hz after load
> >> > ath10k driver, it is because mmc_hw_reset will reset the sdio's power,
> >> > then mmc will consider it as sdio 2.0 and drop the clock.
> >>
> >> Wen,
> >> 5468e784c0600551ca03263f5255a375c05f88e7 commit message gives
> >> reasons
> >> for adding the mmc_hw_reset() call. The commit message for removing
> >> gives different reason for removal. Both are good but second one is
> >> incomplete.
> >>
> >> The commit message for removal should ALSO explain why adding this
> >> call wasn't necessary in the first place OR move the call to a
> >> different code path.
> >>
> >> > Remove mmc_hw_reset will avoid the drop of clock.
> >>
> >> This commit message makes it clear the original patch introduced a new
> >> problem. But the original patch fixed a different problem and that
> >> this proposed change seems likely to re-introduce and the commit
> >> message should explain why that isn't true (or how the original was
> >> fixed differently)
> >
> > The mmc_hw_reset's effect depends on the hardware layout/configure
> > software's behavior, recently it will effect the clock of sdio for the
> > platform I used. And it will still work well without mmc_hw_reset for
> > the platform I Used currently. If sdio cannot work on other platform,
> > I think it can add flag in ath10k_hw_params_list for the platform to
> > call the mmc_hw_reset depends on the flag.
>
> I don't see how you can use ath10k_hw_params_list to separate SDIO
> controller functionality, I assume that's the real reason for difference
> of functionality? Maybe this is a bug on the SDIO controller?

Ideally mmc_hw_reset() should not make the SDIO card to become
re-initialized differently than it was originally.

However, as that is the case here, it sounds like that there is a bug
somewhere. Perhaps the boot-loader have done some pre-initialization,
which isn't covered in the second case, or maybe a bug in the mmc host
driver.

>
> Ulf, what do you think? Any suggestions? Full discussion here:
>
> https://patchwork.kernel.org/patch/10920563/
>
> --
> Kalle Valo

In the end, it seems like this needs a more detailed debug study, to
figure out what exactly happens during the re-initialization of the
SDIO card, rather than just papering over the problem by removing the
call to mmc_hw_reset() in the SDIO func driver. Hope this helps.

Kind regards
Uffe

Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down

[Nicolas Boichat]

On Tue, May 28, 2019 at 9:46 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
[snip]
>
> In the end, it seems like this needs a more detailed debug study, to
> figure out what exactly happens during the re-initialization of the
> SDIO card, rather than just papering over the problem by removing the
> call to mmc_hw_reset() in the SDIO func driver. Hope this helps.

To close the loop on this, we fixed this on the platform by driving a
reset/enable pin during reset:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1657506
(device tree for this device is not upstream yet).

The problem has to do with the fact that on re-init (without power
cycle or reset/enable pin cycling), the device still sets S18A=1 in
CMD5 response (that's incorrect, the device should set S18A=0 if it's
already using 1.8V), so the host tries to switch voltage using CMD11,
which fails, as the device is already in 1.8V mode (that's correct
according to the specs).

Thanks,

>
> Kind regards
> Uffe