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

[Alexey Brodkin <Alexey.Brodkin@synopsys.com>]

On 02/06/2013 01:00 AM, Benjamin Herrenschmidt wrote:
> 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.

Sorry, saying "data access" here I meant accessing 16-bit registers 
indeed. Because most of configuration/control is done via setting values 
in registers. I should have selected proper terms.

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

Sounds good but how should one tell which approach is correct? For 
example here - is the one implemented by Xilinx is golden reference or not?

>>>>> 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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
-Alexey