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