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

[Alexey Brodkin]

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.
--
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/