Messages in this thread | | | From | "Luis R. Rodriguez" <> | Date | Fri, 18 Mar 2011 16:52:08 -0700 | Subject | Re: [RFC 1/4] sdhci: allow for multiple delays when sending commands |
| |
On Fri, Mar 18, 2011 at 4:10 PM, Chris Ball <cjb@laptop.org> 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@laptop.org> >> Cc: Kalle Valo <kvalo@adurom.com> >> Cc: Naveen Singh <nsingh@atheros.com> >> Cc: Vipin Mehta <Vipin.Mehta@atheros.com> >> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> > > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |