Re: [PATCH v18 6/7] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 7/4/2018 9:06 AM, Fabio Estevam wrote:
>> Logan, you mentioned the following (which unfortunately I somehow missed):
>> https://lore.kernel.org/lkml/c3f2e061-5ed1-5c74-b955-3d2bfb0da074@xxxxxxxxxxxx
>> The lo_hi/hi_lo functions seem to always refer to the data being written
>> or read not to the address operated on.

Oy, yes that was more than a year ago.

>> OTOH, initial commit that added asm-generic/io-64-nonatomic-lo-hi.h and
>> asm-generic/io-64-nonatomic-hi-lo.h mentions:
>> 797a796a13df6 ("asm-generic: architecture independent readq/writeq for 32bit
>> environment")
>> - <asm-generic/io-64-nonatomic-lo-hi.h> provides non-atomic readq/ writeq with
>> the order of lower address -> higher address
>> - <asm-generic/io-64-nonatomic-hi-lo.h> provides non-atomic readq/ writeq with
>> reversed order

Hmm, well in fairness that commit didn't add any BE operations so
lower/higher address is the same as lower/higher data being written.
hi-lo/lo-hi is a bit ambiguous in that sense and designing it to match
the semantics of the only user seemed to make sense at the time. I
didn't even check the rough comments in an old commit message which I
wouldn't really take as canonical. Also, it seems to me, most hardware
would expect you to write in order of the address (if it cares at all)
not in the order of the higher/lower data word. Though, I have no
explicit examples only a gut feeling.

>> I think we should keep the initial semantics when adding support for
>> io{read|write}64, i.e. "lo" and "hi" to refer to address and not to value.
>>
>> Actually this is the semantics intended for the CAAM patch, see the note at the
>> end of the commit message that refers to addresses, not values:
>> To be consistent with CAAM engine HW spec: in case of 64-bit registers,
>> irrespective of device endianness, the lower address should be read from
>> / written to first, followed by the upper address.
>>
>>
>> 2. CAAM driver I/O accessors for i.MX case
>> Since this quirk cannot be accommodated in generic fashion, code dealing with
>> caam_imx has to stay.

Yes, though IMO, I think the patch I sent earlier is clearer than the
current code with the nested ifs in different static functions. In any
case, I'll drop it and let you make any changes you see fit.

Logan