[DRIVER UPDATE] aty128fb

[Benjamin Herrenschmidt]

On Thu, Jan 27, 2000, Jes Sorensen <Jes.Sorensen en cern.ch> wrote:

>First of all you ought to use wmb() and rmb() which are the portable
>names for those instructions rather than the PPC specific eieio()
>macro.

rmb() is broken on PPC. Not really broken, but it does a sync, which is a
_huge_ overkill and which should be avoided except when interrupt synchro
or SMP synchro must be acheived.

>Second, you are not adding any io barrier to the 32 bit store macro
>(aty_st_le32()) but you do for the 8 bit version, why?

Hum. It should be there. Did I miss this when you asked my about your
patch, Brad ? I Need some more sleep ...

More simply, I beleive you can directly use readl/writel instead of using
your own asm accessors. (Yours allow to use the index regiser. So they
may actually be good for perfs).

>You also add the io barriers before the operation you are about to
>perform instead of after. That means that if you are storing a value
>to register, you have no idea when it is actually being propagated out
>to the hardware. The safer way to do this would be to put a wmb()
>after each register write and an rmb() after each register read(),
>instead of before

I don't completely agree. Putting them before or after is a matter of
style. The rare cases where this actually matters must usually be
hand-tuned. (For example, filling some HW cursor datas in the
framebuffer, so without using eieio, and then writing a register that
will cause the chip to use those datas. One eieio is needed before the
write in this case. I tend personally to prefer having them before.

Again, rmb() is evil on PPC since it does a sync, which is, I think, very
expensive on recent PPC CPUs.

>Last, this is more of a question, why does the PPC version of the
>driver use it's own inline asm version for the 32 access instead of
>using the readl/writel macros? readl/writel are defined to access the
>PCI bus shared memory in the bus native byte order, ie. little
>endian. Is this because it is running the chip in big endian mode? If
>you want to access it in the hos CPU's native byte order you should
>use the __raw_{write,read}l() macros instead and then add the
>appropriate [rw]mb() macro calls youself.

Optimisation ? Doing so this way allow to use the 2 parameters of the
stwbrx/lwbrx instrutions for the base and the index. That means that the
compiler can store the register base address in one register, and keep it
there all the time, changing only the offset in a second register.

>Note that standard {read,write}[bwl]() are guaranteeing ordering
>themselves so when using those you should not need to add eieio()
>manually. (note this was not the case on the Alpha until some time
>during 2.3.x).

Right.

Ben.



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo en vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/