Date:   Thu, 22 Sep 2022 11:36:13 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Valentin Korenblit <vkorenblit@...uans.com>
Cc:     Arnd Bergmann <arnd@...db.de>, 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 Valentin,

vkorenblit@...uans.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@...db.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

Powered by blists - more mailing lists