Messages in this thread | | | Date | Thu, 7 Feb 2013 16:09:24 +0400 | From | Alexey Brodkin <> | Subject | Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be) |
| |
On 02/07/2013 01:35 AM, Benjamin Herrenschmidt wrote: > 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 I've just realized that in case if there's no bridge between CPU and CF-controller or if this bridge is "transparent" (does no swapping neither bytes nor bits) our data accessors here should be changed.
Isn't it strange in "ace_datain_le16" use "ioread16be" or the one it was here initially "in_be16"? With BE ones I'd say similar changes should be done.
So finally I see them implemented this way: =============== /* BE part */ static void ace_datain_be16(struct ace_device *ace) { int i = ACE_FIFO_SIZE / 2; u16 *dst = ace->data_ptr; while (i--) *dst++ = ioread16be(ace->baseaddr + 0x40); ace->data_ptr = dst; }
static void ace_dataout_be16(struct ace_device *ace) { int i = ACE_FIFO_SIZE / 2; u16 *src = ace->data_ptr; while (i--) iowrite16be(*src++, ace->baseaddr + 0x40); ace->data_ptr = src; }
/* LE part*/ static void ace_datain_le16(struct ace_device *ace) { int i = ACE_FIFO_SIZE / 2; u16 *dst = ace->data_ptr; while (i--) *dst++ = ioread16(ace->baseaddr + 0x40); ace->data_ptr = dst; }
static void ace_dataout_le16(struct ace_device *ace) { int i = ACE_FIFO_SIZE / 2; u16 *src = ace->data_ptr; while (i--) iowrite16(*src++, ace->baseaddr + 0x40); ace->data_ptr = src; } ===============
Correct me if I'm wrong here.
And at least these accessors for LE got xsysace perfectly working on our FPGA platform (little-endian ARC700 on Xilinx ml-509 with our own BVCI-to-MPU bridge that does no swapping).
I have to confess that I didn't properly tested initial patch on real HW - it was only sort of cosmetic clean-up.
-Alexey
>> 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. > >
| |