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

From: Arnd Bergmann
Date: Wed Sep 21 2022 - 11:57:41 EST


On Wed, Sep 21, 2022, at 4:47 PM, Miquel Raynal wrote:
>
> The (erased) context of this thread:
> https://lore.kernel.org/llvm/202209210641.MziHAbW7-lkp@xxxxxxxxx/
>
> vkorenblit@xxxxxxxxxxx wrote on Wed, 21 Sep 2022 14:27:47 +0200:
>
>> Hi Miquel,
>>
>> I see that x86_64 doesn't support readsq/writesq because of
>> CONFIG_GENERIC_IOMAP (I was building for arm64). However,
>> __raw_readq/writeq are available and there are a few drivers
>> using them.
>
> I would suggest to rather try using [read|write]sq() to get rid
> of the CONFIG_GENERIC_IOMAP dependency. But it looks like those
> functions are not defined on 32-bit architectures anyway. So if we want
> the driver to compile on both arm and aarch64, it will not be enough.
>
> In practice, I guess we should never have the 64-bit accessor executed
> when running on a 32-bit platform thanks to the following conditions:
>
> 1885 u8 data_dma_width = cdns_ctrl->caps2.data_dma_width;
> 1886
> 1887 int len_in_words = (data_dma_width == 4) ? len >> 2 : len >> 3;
> 1888
> 1889 /* read alingment data */
> 1890 if (data_dma_width == 4)
> 1891 ioread32_rep(cdns_ctrl->io.virt, buf, len_in_words);
> 1892 else
>> 1893 ioread64_rep(cdns_ctrl->io.virt, buf, len_in_words);
>
> So maybe we could have something awful yet simple, like the following
> within the Cadence driver:
>
> #if !CONFIG_64BIT
> readsq(...) { BUG()? }
> #endif
>
> Arnd, I've seen a couple of similar issues on the mailing lists in the
> past 5 years but I could not find any working solution, I don't know
> how all these threads have settled in the end. I thought maybe you
> would have a better idea than the above hack :)

There are a lot of different things going on here, so I need
to unwind a bit:

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

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

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

- On 32-bit architectures, there is no generic way to access a 64-bit
MMIO location as you say. However, depending on the device you are
accessing, you can replace __raw_readq() with a pair of
__raw_readl(), along the lines of
include/linux/io-64-nonatomic-{lo-hi,hi-lo}.h

>> Do you want me to re-implement readsq and writesq in Cadence
>> driver privately or do you suggest a different (cleaner) approach?
>
> I would rather prefer to avoid this solution, as anyway I believe there
> is no "generic" implementation working in all cases, there were a
> couple of attempts IIRC to bring generic helpers like the above for all
> architectures, but none of them landed in Linus' tree, probably because
> it just cannot be made...

If anyone has a datasheet for the cadence driver, they should be
able to look up how one can access the FIFO in 8-byte mode using
32-bit accesses. I think it's something like

static inline void cadence_nand_readsq(const volatile void __iomem *addr,
void *buffer, unsigned int count)
{
if (count) {
u64 *buf = buffer;

do {
#ifdef __raw_readq
u64 x = __raw_readq(addr);
*buf++ = x;
#else
u32 *buf32 = (void *)buf;
buf32[0] = __raw_readl(addr + OFF0);
buf32[1] = __raw_readl(addr + OFF1);
buf++;
#endif
} while (--count);
}
}

Most likely, OFF0 is zero, while OFF1 is 4 here, but that
depends on how the FIFO register is implemented.

Arnd