Hi Arnd, Miquel,

On Wed, Sep 21, 2022, at 6:38 PM, Miquel Raynal wrote:

arnd@arndb.de wrote on Wed, 21 Sep 2022 17:49:11 +0200:

On Wed, Sep 21, 2022, at 4:47 PM, Miquel Raynal wrote:

- every architecture should provide readsq()/readsl()/readsw()/readsb()
  these days, regardless of CONFIG_GENERIC_IOMAP. If x86 does
  not have that, we should fix asm-generic/io.h.

ARM does not seem to define readsq/writesq. Should it be fixed?

64-bit Arm should get it from include/asm-generic/io.h. If it does
not, this should be fixed. 32-bit Arm obviously cannot define them
in a generic way.

This is ok for 64-bit arm.

- CONFIG_GENERIC_IOMAP just means an architecture uses the generic
  ioread32_rep() style wrapper around readsl()/insl(). On most
  architectures (not x86), insl() is implemented as a wrapper around
  readsl() itself, so readsl() and ioread32_rep() should be identical.

Ok. But if CONFIG_GENERIC_IOMAP=n (ARM, aarch64, x86_64),

x86_64 has GENERIC_IOMAP=y

ioread64_rep is then only defined if CONFIG_64BIT. As it is based
on readsq/writesq() and those must be defined (as you said), I don't get
why the *64_rep() helpers are not defined in all cases. Maybe because no
32-bit system _should_ need them? But then compile testing gets more
difficult.

Both readsq/writesq and ioread64_rep/iowrite64_rep must be defined
for 64-bit architectures and cannot be defined for 32-bit ones.

So the first issue here is that they are not defined for x86_64
(CONFIG_64BIT=y).

- For a FIFO, you cannot use readq() but have to use __raw_readq()
  to get the correct endianness. You cannot use this for an
  MMIO register with side-effects though, as this needs the byteswap
  and the barrier in readsl().

I'm not sure about the true definition of "FIFO" as you say. I guess
you just mean reading from a BE device?

In this case I guess we need the barrier+byteswap helpers.

The difference is that a register has a fixed length, and gets
accessed with a device specific endianness, which may have to
be swapped if the device and the CPU disagree.

A FIFO register is what you use for transferring a stream of
bytes, such as reading a file system block from disk. The
first byte in the register corresponds to the first byte in
memory later, so there must not be any byteswap while copying
to/from memory. If the data itself is structured (i.e. an
on-disk inode or a network packet), then the byteswap will
happen if necessary while interpreting the data.

I don't think this is actually what we want. My understanding is
(Valentin, please correct me if I'm wrong):
- on ARM: we will always use 32-bit accesses
- on aarch64: we may use either 32-bit or 64-bit accesses
- on other architectures: we only want to compile test

Correct, this was my initial idea. However, this driver should work
with every architecture or do we limit the scope to arm/arm64/x86_64?

I believe what Valentin wanted to achieve in the first place, was to
use 64-bit accesses when relevant (otherwise it does not work).

The width is read from a device specific register at
runtime, it is not related to the architecture you are
running on, presumably this is hardwired during the
design of an SoC, based on the capabilities of the DMA
engine:

        reg = readl_relaxed(cdns_ctrl->reg + CTRL_FEATURES);
        if (FIELD_GET(CTRL_FEATURES_DMA_DWITH64, reg))
                cdns_ctrl->caps2.data_dma_width = 8;
        else
                cdns_ctrl->caps2.data_dma_width = 4;

This usually means the largest access that is valid for
reading from the FIFO, but usually smaller accesses work
as well, just slower.

Thanks for all the info. I can check if consecutive smaller accesses 
trigger sdma_err interrupt. The datasheet only specifies: "if host sends an
unsupported transaction to slave interface, the slave dma ignores
this access and sets sdma_err flag in intr_status register.

Valentin