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

[Benjamin Herrenschmidt]

On Tue, 2013-02-05 at 18:03 +0400, Alexey Brodkin wrote:
> =====
> 
> and its endianess is fixed which is stated in this document 
> (http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf).
> =====
> The Xilinx System ACE Compact Flash chip is a true little-endian device
> =====

So far so good...

> But in its turn Xilinx System ACE Compact Flash chip is attached to 
> CPU's interface bus via some bridge. Depending on interface bus used it 
> could be BVCI-to-MPU (Microprocessor Interface of the System ACE
> Compact Flash solution peripheral) for ARC, PLB for MicroBlaze etc.
> And this bridge in general may do whatever its (HW) developer wants.
> 
> For example for MicroBlaze Xilinx has its XPS Sysace interface 
> controller 
> (http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf) 
> which does bit-swapping from PLB's big-endian bytes to xsysace 
> CF-controller little-endian bytes (though bytes in words are not swapped):
> =====
> The Xilinx System ACE Compact Flash chip is a true little-endian device 
> and the PLB is a big-endian bus. Therefore the XPS System ACE Interface 
> Controller will do a bit-swap in each byte when connecting the PLB data 
> bus to the System ACE data bus as shown in Table 2.

This sounds totally bogus to me. But I'll have a look at the doco to
understand it better. I'll try to do so later today.

> Note however, that the XPS System ACE Interface Controller does not 
> perform the byte swapping necessary to interface to a little-endian 
> device when configured to use 16-bit mode. Therefore, the software 
> drivers provided for this core will perform the necessary byte-swapping 
> to correctly interface to the Xilinx System ACE Compact Flash chip as 
> shown in Table 3.
> =====
> 
> So at this point I'd say that data access should be done differently 
> depending on HW (bridge) used.

Well, bytes accesses should never need any swapping whatsoever. 16-bit
access requires swapping for a LE device for register, never for a data
port. If it does, the bridge is wired incorrectly.

> Another question is do we know for sure that for particular architecture 
> only 1 interface bridge is used. If so then we may select proper 
> accessors per architecture.

There aren't two ways to wire up an interface bridge correctly. I would
advocate not supporting any incorrect wiring in Linux. Doing so would be
going back to supporting horrors like IDE wired backward etc... which we
have mostly gotten rid of.

People need to be educated in this area, and Linux upstream doesn't have
to support any piece of shit anybody comes up with because they can't be
bothered understanding what endianness and byte address invariance mean.

> >>> It is just sharing the same IP across all platforms. Which is better
> >>> than create new devices and new device drivers for it. It means that
> >>> all of them are register compatible but require access with native
> >>> platform endianness as I listed above.
> >>
> >> Every attempt at doing "native platform endianness" has always been a
> >> misguided attempt turning into a trainwreck (see OHCI USB).
> >>
> >> Just pick one endian for the device and stick to it.
> >
> > It is reality and I can't change it. Arnd mentioned it earlier that USB
> >
> >
> >>> It is not a problem to create runtime wrapper and even detect endian
> >>> directly in the driver
> >>> but the point if this is the proper design.
> >>> Also ioread32 and ioread32be shouldn't be used on ARM because there
> >>> are missing memory barriers.
> >>
> >> Then fix them, they shouldn't be, it's a bug, it will break many other
> >> drivers. They should be fully equivalent to readl.
> >
> > I want to be sure about this. I have parsed this again with closer look and
> > seems to me that ioread32 is equal to readl and iowrite32 to writel.
> > Arnd: Am I right?
> >
> > Thanks,
> > Michal
> 
> -Alexey
> --
> 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/


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