On Fri, Mar 18, 2011 at 4:10 PM, Chris Ball <cjb@xxxxxxxxxx> wrote: > Hi Luis, > > On Fri, Mar 18 2011, Luis R. Rodriguez wrote: >> This is at least known to be required for the ENE 714. >> >> Cc: Chris Ball <cjb@xxxxxxxxxx> >> Cc: Kalle Valo <kvalo@xxxxxxxxxx> >> Cc: Naveen Singh <nsingh@xxxxxxxxxxx> >> Cc: Vipin Mehta <Vipin.Mehta@xxxxxxxxxxx> >> Signed-off-by: Luis R. Rodriguez <lrodriguez@xxxxxxxxxxx> > > I think this wins cjb's "I've never been so confused about what a patch > author thought they were doing before" award. I warned you :) >> --- >> Âdrivers/mmc/host/sdhci.c | Â Â7 ++++--- >> Â1 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index 9e15f41..c95dfc2 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -891,8 +891,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >> >> Â Â Â WARN_ON(host->cmd); >> >> - Â Â /* Wait max 10 ms */ >> - Â Â timeout = 10; >> + Â Â /* Wait max this amount of ms */ >> + Â Â timeout = (10*256) + 255; >> >> Â Â Â mask = SDHCI_CMD_INHIBIT; >> Â Â Â if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY)) > > Okay, so our original plan is to go through the while loop ten times, > which equals ten mdelay(1)s == 10ms, waiting for the inhibit bit to > become unset. > > After this hunk of your patch, we're set to go through the loop 2815 > times, which would make for 2.8 seconds. ÂThat seems excessive. > >> @@ -913,7 +913,8 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd) >> Â Â Â Â Â Â Â Â Â Â Â return; >> Â Â Â Â Â Â Â } >> Â Â Â Â Â Â Â timeout--; >> - Â Â Â Â Â Â mdelay(1); >> + Â Â Â Â Â Â if (!(timeout & 0xFF)) >> + Â Â Â Â Â Â Â Â Â Â mdelay(1); >> Â Â Â } >> >> Â Â Â mod_timer(&host->timer, jiffies + 10 * HZ); > > But wait, here you decide *not* to call mdelay(1) every time through the > loop, and instead call it only on iterations where the bottom eight bits > are unset. ÂThis disqualifies most of the 2815 values that timeout will > be set to, and leaves the following values triggering the mdelay(1): > > 0x100 0x200 0x300 0x400 0x500 0x600 0x700 0x800 0x900 0xa00 > Â256 Â 512 Â 768 Â1024 Â1280 Â1536 Â1792 Â2048 Â2304 Â2560 > > The astute observer will notice that there are ten such values. > So you're calling mdelay(1) ten times. ÂBut that's what we were > doing before! ÂThe only difference is that now we spin through > the while loop 2815 times instead of 10, and don't perform any > explicit delay on 2805 of them. ÂOr am I missing something? > > I think you should try: > > (a) Reverting the patch and checking that it's actually needed > (b) Leaving the while loop body alone, but increasing the max > Â Âtimeout until you bisect to the amount of ms that your controller > Â Âactually takes to release the inhibit bit. > (c) Yelling loudly in the direction that this code came from. Â:) Heh thanks for the review, once I even get ath6kl to even load properly on x86_64 I'll try reverting this POS hunk and see if it still works without it. Who knows where the hell this patch came from, as I noted I was given the entire blob as a huge patch and if you look at it, the original patch even removes some quirk for tegra. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
