Re: [PATCH v6 2/2] clocksource: add J-Core timer/clocksource driver

[Mark Rutland]

On Wed, Aug 24, 2016 at 04:52:26PM -0400, Rich Felker wrote:
> On Wed, Aug 24, 2016 at 10:01:08PM +0200, Arnd Bergmann wrote:
> > On Wednesday, August 24, 2016 1:40:01 PM CEST Rich Felker wrote:
> > > I actually tried multiple times to find good resources on policy for
> > > which form to prefer, but didn't have much luck. My understanding is
> > > that __raw_writel/__raw_readl always performs a native-endian
> > > load/store, whereas writel/readl behavior depends on cpu endianness
> > > and whether the arch declares that "pci bus" (that was the term I
> > > found in the source, not relevant here) io is endian-swapped or not.
> > > Can you give me a better explanation of why we might prefer one form
> > > or the other?
> > 
> > In general, a portable driver should use readl/writel because they
> > are defined to have the behavior that x86 provides, which is what
> > most developers understand best. Compared to __raw_readl/__raw_writel,
> > it guarantees
> > 
> > - correct endianess (in most cases, exceptions see below)
> > - does not leak out of spinlocks
> 
> Not even normal non-volatile accesses can leak across spinlocks;
> they're memory+compiler barriers.

Note that Arnd was talking in general, about cross-architectural guarantees.

Across architectures, not all barriers are equivalent. On some architectures,
the barriers used by spinlocks don't necessarily order accesses across
disparate memory types (i.e. DRAM and IO), or might not guarantee that the
ordering is consistent to observers other than (coherent) CPUs. Hence a
portable driver should care about this distinction.

If they're equivalent for you, you may as well use the portably stronger form,
as it's less likely to surprise others.

> > - accesses are word-sized, the compiler cannot split them into
> >   byte accesses, or merge subsequent accesses into larger words
> 
> Based on my reading of the code that's also true for the raw ones. At
> least GCC interprets volatile as disallowing split/combined
> loads/stores when the hardware supports the nominal load/store size,
> and this seems to agree with the intent of the C standard that
> volatile be usable for memory-mapped devices..
> 
> > - ordering against dma_map_*/dma_unmap_*/dma_sync_* is maintained
> 
> Aren't they barriers too?

As above, not all barriers are equivalent. Your architecture may provide
stronger (or weaker) guarantees than others.

> > The __raw_* accessors are used as building blocks for
> > readl/outl/ioread32/ioread32_be/readl_relaxed/... and they can
> > usually be used safely on device RAM such as framebuffers but
> > not much else in portable code. Some architectures also use the
> > __raw_* accessors for MMIO registers, but that code is generally
> > not portable.
> 
> My concern is that, depending on some arcane defines, readl/writel
> could start doing byte-reversal of values since the CPU is big-endian.
> arch/sh seems to have them defined in such a way that this doesn't
> happen, but I was hoping to avoid depending on that. If you think it's
> safe to assume they do the right thing, though, I'm okay with it. If
> anything breaks when we try a little-endian version of the CPU/SoC
> later I can go back and fix it.

I guess this really depends on the expectations you have w.r.t. the endianness
of devices vs the endianness of CPUs. If you expect the two to always be
matched, please add a comment in the driver to that effect.

As Arnd and others mentioned, the vastly common case is that the endianness of
CPUs and devices is not fixed to each other, and devices are typically
little-endian.

> > Endianess is always more complicated than one thinks, and it's
> > handled differently across architectures, sometimes because of
> > hardware differences, sometimes for historic reasons that differ
> > only in the Linux implementation.
> > 
> > A large majority of devices are fixed to little-endian MMIO
> > registers (as recommended by PCI), so that is a good default.
> > 
> > Exceptions to this are:
> > 
> > * Some architectures (e.g. PowerPC) typically run big-endian code,
> >   and hardware designers decided to make the MMIO registers the
> >   same. In this case, you can generally use ioread32_be/iowrite32_be.
> 
> This is something like our case, but the mmio registers are accessed
> as 32-bit values and the endianness does not come into play. 

This really depends on you endianness model, bus, etc. The above sounds like
BE32 rather than BE8, assuming I've understood correctly, which is an unusual
case nowadays.

Thanks,
Mark.