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

[Valentin Korenblit <vkorenblit@sequans.com>]

[-- Attachment #1: Type: text/plain, Size: 4531 bytes --]

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

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

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

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
-- IMPORTANT NOTICE:



The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.



Thank you.

[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 6824 bytes --]