Re: [PATCH 06/12] drm/nouveau/ibus: add GK20A support

[Alexandre Courbot]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/