[Nouveau] [PATCH 06/12] drm/nouveau/ibus: add GK20A support
On Wed, Apr 2, 2014 at 11:18 PM, Ilia Mirkin wrote: > On Wed, Apr 2, 2014 at 9:52 AM, Alexandre Courbot wrote: >> On Tue, Mar 25, 2014 at 7:34 AM, Thierry Reding >> 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 + +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. > > I just remembered about this, but there's also the nv_wait() macro, > which you could use, e.g. > > if (!nv_wait(subdev, 0x12004c, 0x3f, 0x00)) > nv_warn(...) > > It has built-in timeout logic/etc (although no sleeps in the middle). > It does use the timer subdev, so if that's not operational at this > point, you can't use it. IBUS comes after TIMER in the nv_subdev_type enum, so I guess that should work. Will try this solution, thanks!
[Nouveau] [PATCH 06/12] drm/nouveau/ibus: add GK20A support
On Wed, Apr 2, 2014 at 9:52 AM, Alexandre Courbot wrote: > On Tue, Mar 25, 2014 at 7:34 AM, Thierry Reding > 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 >>> + >>> +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. I just remembered about this, but there's also the nv_wait() macro, which you could use, e.g. if (!nv_wait(subdev, 0x12004c, 0x3f, 0x00)) nv_warn(...) It has built-in timeout logic/etc (although no sleeps in the middle). It does use the timer subdev, so if that's not operational at this point, you can't use it. > >> 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. > ___ > Nouveau mailing list > Nouveau at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau