Re: [mtd:nand/next 11/31] drivers/mtd/nand/raw/cadence-nand-controller.c:1893:4: error: implicit declaration of function 'ioread64_rep' is invalid in C99

[Arnd Bergmann]

On Wed, Sep 21, 2022, at 6:38 PM, Miquel Raynal wrote:
> arnd@xxxxxxxx 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.

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

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

     Arnd