Message-ID: <20220921183807.241e2518@xps-13>
Date:   Wed, 21 Sep 2022 18:38:07 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     "Arnd Bergmann" <arnd@...db.de>
Cc:     "Valentin Korenblit" <vkorenblit@...uans.com>,
        "kernel test robot" <lkp@...el.com>, llvm@...ts.linux.dev,
        kbuild-all@...ts.01.org, linux-kernel@...r.kernel.org
Subject: 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

Hi Arnd,

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

> 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@intel.com/
> >
> > vkorenblit@...uans.com 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:

Thanks for all your feedback!

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

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

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

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

Ah ok, so in the end the per-driver implementation is preferred.

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

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

> 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


Thanks,
Miquèl

Powered by blists - more mailing lists