Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2013-02-06 at 10:14 +0000, Grant Likely wrote:
> 
> Huh? That makes no sense. This device out in the wild with both big
> and little endian bus attachments. You can argue all day that one of
> them is wrong, but it doesn't matter. It exists, is used, and must be
> supported.

No. That's where you are VERY wrong. We don't have to support crap and
arguably shouldn't if that can give an incentive to vendors to fix their
stuff. If you don't believe me, ask Linus :-)

>  In fact, the driver already knows about this and figures
> out at runtime how the device is wired up to the bus. This is not the
> problem.

Except that this is very gross, especially when you observe that in the
busted "big endian" case, it has to byteswap the bloody data port.

So you end up having to do that gross hack with separate accessors for
registers vs. data and not able to use the _rep variants, which also
means that on platforms like ppc, you end up with a memory barrier on
every access (or more), which is going to slow things down enormously.

> BTW, that document describes only one of the systemace bus
> attachments. There is a different on used on Microblaze little-endian,
> and some boards have the SystemACE directly wired to the SoC external
> bus (no adapter IP).
> 
> The only problem that I see is that the ARM and Microblaze
> ioread16be/iowrite16be helpers are missing barriers which smells like
> a bug and should be fixed.
> 
> Michal, have you tested Alexey's patch? If it works for you then I'm
> comfortable with acking it. It looks correct to me.

No, the real problem is that the "big endian" wiring is totally busted
and the HW guys who came with it must be taught a lesson. Not supporting
that crap might be one way to do it.

Ben.