write ordering and macfb nop() usage

write ordering and macfb nop() usage

[Finn Thain]

Hi All,

I've been working on macfb.c, and I notice that it is full of stuff like 
this:

        /* the nop's are there to order writes. */
        nubus_writeb(_regno, &cmap_regs->addr); nop();
        nubus_writeb(_red, &cmap_regs->lut);    nop();
        nubus_writeb(_green, &cmap_regs->lut);  nop();
        nubus_writeb(_blue, &cmap_regs->lut);

Timing aside, I don't see what are the nop's are for. I thought accessors 
like nubus_write/raw_out/out are intended to issue these writes in order 
anyway?

And why were nop's were used instead of an empty asm() or just a volatile 
qualifier anyway? Some accident of history like a compiler bug?

Also, can a nop provide a useful timing constraint when bus speed can vary 
so much from one machine to another?

I'm baffled.

Finn

Re: write ordering and macfb nop() usage

[Brad Boyer]

On Wed, Oct 15, 2008 at 03:56:35AM +1100, Finn Thain wrote:
> I've been working on macfb.c, and I notice that it is full of stuff like 
> this:
> 
>         /* the nop's are there to order writes. */
>         nubus_writeb(_regno, &cmap_regs->addr); nop();
>         nubus_writeb(_red, &cmap_regs->lut);    nop();
>         nubus_writeb(_green, &cmap_regs->lut);  nop();
>         nubus_writeb(_blue, &cmap_regs->lut);
> 
> Timing aside, I don't see what are the nop's are for. I thought accessors 
> like nubus_write/raw_out/out are intended to issue these writes in order 
> anyway?

I would expect the accessor functions to do the right thing, but I would
not be surprised if they don't. The NuBus code is pretty hackish in general
due to lack of documentation of all the implementations. This reminds me
of the via memory bogon stuff that used to be in via.c. Some of the early
mac code is very sloppy. The basic nubus.c code could use some attention.
It's on my list, but that list keeps getting longer and this one isn't
that close to the top of my list.

> And why were nop's were used instead of an empty asm() or just a volatile 
> qualifier anyway? Some accident of history like a compiler bug?

My guess is that this was easier in some way.

> Also, can a nop provide a useful timing constraint when bus speed can vary 
> so much from one machine to another?

No, it can't. The bus interface circuits are supposed to do the clock sync
for us in any case. It's possible that we really should be reading some
kind of status from the card and this delay just happens to make it work
most of the time.

> I'm baffled.

I'd think you'd be used to that by now.  :)

	Brad Boyer
	flar@allandria.com

Re: write ordering and macfb nop() usage

[Finn Thain]

On Tue, 14 Oct 2008, Brad Boyer wrote:

> On Wed, Oct 15, 2008 at 03:56:35AM +1100, Finn Thain wrote:

> I would expect the accessor functions to do the right thing, but I would 
> not be surprised if they don't.

Well, if the volatile qualifier doesn't provide write ordering, then the 
next question would be, do we need a barrier in nubus_read/write?

Or are you suggesting that the cache might re-order these writes? NuBus 
frame buffers don't have CLUT registers ioremap'd in macfb.c, but the 
built-in frame buffers do. Presumably that's because nubus slot space is 
already mapped PAGE_NOCACHE by head.S.

> ...The basic nubus.c code could use some attention.

Not sure what you have in mind? I notice that there was some work done 
there for Linux/ppc-nubus*...

I've been working on nubus.c too, since it has a bug that seems to 
register cards twice (causing a stack trace from the procfs driver and 
also causing macfb to match the same card twice). And there's some patches 
from the mac68k repo that I've forward-ported. And the usual minor fixes 
and cleanups.

> It's possible that we really should be reading some kind of status from 
> the card and this delay just happens to make it work most of the time.

I did some digging in the mac68k CVS. It looks like the nops first 
appeared in the code for TFB and MDC support** ("to order writes"), and 
were simultaneously added for RBV. (TFB and MDC are nubus, RBV isn't of 
course.) Then in the next commit nops were added to all other pallete 
routines with no reason given. (The other routines, like RBV, were 
apparently already working at that point.)

I guess I'll just remove the nops and test the palette code.

Finn


* http://nubus-pmac.cvs.sourceforge.net/viewvc/nubus-pmac/linuxppc-2.4-nubus/drivers/nubus/nubus.c?r1=1.5&r2=1.6
** http://linux-mac68k.cvs.sourceforge.net/viewvc/linux-mac68k/linux-mac68k/drivers/video/macfb.c?r1=1.19&r2=1.20&pathrev=MAIN

Re: write ordering and macfb nop() usage

[Brad Boyer]

On Thu, Oct 16, 2008 at 02:01:37AM +1100, Finn Thain wrote:
> On Tue, 14 Oct 2008, Brad Boyer wrote:
> > I would expect the accessor functions to do the right thing, but I would 
> > not be surprised if they don't.
> 
> Well, if the volatile qualifier doesn't provide write ordering, then the 
> next question would be, do we need a barrier in nubus_read/write?

The volatile qualifier just prevents certain types of reordering by
the compiler. However, the 68k chips and early Mac hardware were
generally simple enough to not have the same runtime ordering issues
as more modern architectures. It's probably enough for us, but I
doubt it's enough for ppc systems with NuBus. I will note that they
seem to have added some barriers in their version of nubus.c.

> Or are you suggesting that the cache might re-order these writes? NuBus 
> frame buffers don't have CLUT registers ioremap'd in macfb.c, but the 
> built-in frame buffers do. Presumably that's because nubus slot space is 
> already mapped PAGE_NOCACHE by head.S.

We really shouldn't rely on that in the generic NuBus code, particularly
since that puts an extra burden on the ppc people whenever they get their
code working again.

> > ...The basic nubus.c code could use some attention.
> 
> Not sure what you have in mind? I notice that there was some work done 
> there for Linux/ppc-nubus*...

Moving to the modern device/driver model would be nice. This would let
us integrate much better with the rest of the kernel. Removing some of
the m68k centric assumptions would be good, too.

> I've been working on nubus.c too, since it has a bug that seems to 
> register cards twice (causing a stack trace from the procfs driver and 
> also causing macfb to match the same card twice). And there's some patches 
> from the mac68k repo that I've forward-ported. And the usual minor fixes 
> and cleanups.

That's always a good thing, although it's separate from what I was
considering. We obviously need to fix bugs.

> > It's possible that we really should be reading some kind of status from 
> > the card and this delay just happens to make it work most of the time.
> 
> I did some digging in the mac68k CVS. It looks like the nops first 
> appeared in the code for TFB and MDC support** ("to order writes"), and 
> were simultaneously added for RBV. (TFB and MDC are nubus, RBV isn't of 
> course.) Then in the next commit nops were added to all other pallete 
> routines with no reason given. (The other routines, like RBV, were 
> apparently already working at that point.)

My guess is that someone just tried to make them all look alike without
understanding why they were there in the other code. What we may actually
need is just a really short delay. Some of the mac support code already
does that sort of thing for hardware that takes time to settle that isn't
handled automatically by the bus interface logic.

> I guess I'll just remove the nops and test the palette code.

It's worth trying. If there's hardware you don't have but want to test,
I do have a fair number of systems laying around. Maybe someone else
will be where they could test other hardware as well. I have some Toby
video cards, but not any of the other supported NuBus video cards. I
also have a couple unsupported NuBus video cards that just make Linux
not boot at all.

	Brad Boyer
	flar@allandria.com