On Tue, Mar 25, 2014 at 7:34 AM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > On Mon, Mar 24, 2014 at 05:42:28PM +0900, Alexandre Courbot wrote: > [...] >> diff --git a/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c b/drivers/gpu/drm/nouveau/core/subdev/ibus/nvea.c > [...] >> +#include <subdev/ibus.h> >> + >> +struct nvea_ibus_priv { >> + struct nouveau_ibus base; >> +}; >> + >> +static void >> +nvea_ibus_init_priv_ring(struct nvea_ibus_priv *priv) >> +{ >> + nv_mask(priv, 0x137250, 0x3f, 0); >> + >> + nv_mask(priv, 0x000200, 0x20, 0); >> + udelay(20); > > usleep_range()? Sure. > >> +static void >> +nvea_ibus_intr(struct nouveau_subdev *subdev) >> +{ > [...] >> + /* Acknowledge interrupt */ >> + nv_mask(priv, 0x12004c, 0x2, 0x2); >> + >> + while (--retry >= 0) { >> + command = nv_rd32(priv, 0x12004c) & 0x3f; >> + if (command == 0) >> + break; >> + } >> + >> + if (retry < 0) >> + nv_warn(priv, "timeout waiting for ringmaster ack\n"); >> +} > > Perhaps I'm being paranoid, but this loop now depends on the frequency > of the various clocks involved and therefore might break at some point > if the frequencies get sufficiently high. > > So a slightly safer implementation would use a proper timeout using a > combination of msecs_to_jiffies(), time_before() and usleep_range(), > like so: > > timeout = jiffies + msecs_to_jiffies(...); > > while (time_before(jiffies, timeout)) { > command = nv_rd32(...) & 0x3f; > if (command == 0) > break; > > usleep_range(...); > } > > if (time_after(jiffies, timeout)) > nv_warn(...); Right, now that I look at this code again I don't even understand why I left it this way. Maybe I left some early test code slip into the final patch, sorry about that. > This assumes that there's some known timeout after which the ringmaster > is expected to have acked the interrupt. On that note, I wonder if the > warning is accurate here: it's my understanding that writing 0x2 to the > register does acknowledge the interrupt, so the ringmaster does in fact > "clear" it rather than "acknowledge" it, doesn't it? > > Although now that I mention it I seem to remember that this write is > actually sending a command to the ring master and perhaps waiting for > the register to return to 0 is indeed waiting for an ACK of sorts. Maybe > adding a comment or so describing what this sequence does would be > appropriate here? Can we from an IP point of view? AFAIK this sequence has never been publicly documented. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html