| Message ID | 1556417825-13713-1-git-send-email-wgong@codeaurora.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: remove mmc_hw_reset while hif power down | expand |
[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 >
> -----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
+ 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/
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
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
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;
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(-)