On Tue, 7 May 2019 at 11:35, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote: > > + Ulf to give comments from SDIO point of view Apologize for the delay, it's been a busy period. > > Wen Gong <wgong@xxxxxxxxxxxxxxxx> writes: > > >> -----Original Message----- > >> From: ath10k <ath10k-bounces@xxxxxxxxxxxxxxxxxxx> On Behalf Of Grant > >> Grundler > >> Sent: Saturday, May 4, 2019 2:01 AM > >> To: Wen Gong <wgong@xxxxxxxxxxxxxx> > >> Cc: linux-wireless@xxxxxxxxxxxxxxx; ath10k@xxxxxxxxxxxxxxxxxxx > >> 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@xxxxxxxxxxxxxx> 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
