* [PATCH] ath10k: remove mmc_hw_reset while hif power down @ 2019-04-28 2:17 Wen Gong 2019-05-03 18:01 ` Grant Grundler 0 siblings, 1 reply; 6+ messages in thread From: Wen Gong @ 2019-04-28 2:17 UTC (permalink / raw) To: ath10k; +Cc: linux-wireless 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down 2019-04-28 2:17 [PATCH] ath10k: remove mmc_hw_reset while hif power down Wen Gong @ 2019-05-03 18:01 ` Grant Grundler 2019-05-07 5:05 ` Wen Gong 0 siblings, 1 reply; 6+ messages in thread From: Grant Grundler @ 2019-05-03 18:01 UTC (permalink / raw) To: Wen Gong; +Cc: ath10k, linux-wireless [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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] ath10k: remove mmc_hw_reset while hif power down 2019-05-03 18:01 ` Grant Grundler @ 2019-05-07 5:05 ` Wen Gong 2019-05-07 9:34 ` Kalle Valo 0 siblings, 1 reply; 6+ messages in thread From: Wen Gong @ 2019-05-07 5:05 UTC (permalink / raw) To: Grant Grundler, Wen Gong Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org > -----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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down 2019-05-07 5:05 ` Wen Gong @ 2019-05-07 9:34 ` Kalle Valo 2019-05-28 12:45 ` Ulf Hansson 0 siblings, 1 reply; 6+ messages in thread From: Kalle Valo @ 2019-05-07 9:34 UTC (permalink / raw) To: Wen Gong Cc: Grant Grundler, Wen Gong, linux-wireless@vger.kernel.org, ath10k@lists.infradead.org, Ulf Hansson + 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down 2019-05-07 9:34 ` Kalle Valo @ 2019-05-28 12:45 ` Ulf Hansson 2019-06-18 10:39 ` Nicolas Boichat 0 siblings, 1 reply; 6+ messages in thread From: Ulf Hansson @ 2019-05-28 12:45 UTC (permalink / raw) To: Kalle Valo, Wen Gong, Grant Grundler Cc: Wen Gong, linux-wireless@vger.kernel.org, ath10k@lists.infradead.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath10k: remove mmc_hw_reset while hif power down 2019-05-28 12:45 ` Ulf Hansson @ 2019-06-18 10:39 ` Nicolas Boichat 0 siblings, 0 replies; 6+ messages in thread From: Nicolas Boichat @ 2019-06-18 10:39 UTC (permalink / raw) To: Ulf Hansson Cc: Kalle Valo, Wen Gong, Grant Grundler, Wen Gong, linux-wireless@vger.kernel.org, ath10k@lists.infradead.org, Claire Chang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-18 10:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-04-28 2:17 [PATCH] ath10k: remove mmc_hw_reset while hif power down Wen Gong 2019-05-03 18:01 ` Grant Grundler 2019-05-07 5:05 ` Wen Gong 2019-05-07 9:34 ` Kalle Valo 2019-05-28 12:45 ` Ulf Hansson 2019-06-18 10:39 ` Nicolas Boichat
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).