Messages in this thread | | | Date | Thu, 22 Sep 2022 11:36:13 +0200 | From | Miquel Raynal <> | 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 Valentin,
vkorenblit@sequans.com wrote on Thu, 22 Sep 2022 10:18:46 +0200:
> Hi Arnd, Miquel, > > On 9/21/22 22:01, Arnd Bergmann wrote: > > On Wed, Sep 21, 2022, at 6:38 PM, Miquel Raynal wrote: > >> 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: > >>> - 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. > > This is ok for 64-bit arm. > > > > >>> - 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. > > So the first issue here is that they are not defined for x86_64 > (CONFIG_64BIT=y).
I would say this is not very important as long as we could use readsq/writesq() for the same purpose.
> >>> - 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 > > Correct, this was my initial idea. However, this driver should work > with every architecture or do we limit the scope to arm/arm64/x86_64?
The driver should work on ARM and aarch64, I'm not aware of other architectures with this IP.
The driver should compile when COMPILE_TEST=y.
> >> 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:
Well, yes, but in the mean time 64-bit DMA width will never be used on 32-bit platforms.
> > 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.
Mmh, ok, that's interesting, thanks for the pointer.
But in the mean time I am only half satisfied, because we plan to do twice more accesses than needed _just_ because of a the COMPILE_TEST constraint.
> Thanks for all the info. I can check if consecutive smaller accesses > trigger sdma_err interrupt. The datasheet only specifies: "if host sends an > unsupported transaction to slave interface, the slave dma ignores > this access and sets sdma_err flag in intr_status register. > > Valentin
Thanks, Miquèl
| |