lkml.org 
[lkml]   [2022]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@arndb.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@sequans.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

\
 
 \ /
  Last update: 2022-09-21 18:47    [W:0.057 / U:0.996 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site