Re: [PATCH] dt-bindings: misc: Add binding for Xilinx AXI-Stream FIFO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] DT Binding ABI Breakage and Duplicate Documentation
- [High] IRQ Race Condition Dropping Unhandled Interrupts
- [High] Incorrect MMIO Accessor Semantics for FIFOs
- [High] Missing Clock Definition and Enablement
--

commit 31b86c7f3095299d0d2eedef1065b0b06442005b
Author: Pramod Maurya <pramod.nexgen@gmail.com>

dt-bindings: misc: Add binding for Xilinx AXI-Stream FIFO

Add a YAML schema for the Xilinx AXI-Stream FIFO IP core (PG080).
The binding documents the three supported compatible strings and all
vendor-specific properties that the axis-fifo driver reads from the
device tree.

> diff --git a/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml
> new file mode 100644
> index 0000000000000..1e07db9cd4565
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/xlnx,axi-fifo-mm-s.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/xlnx,axi-fifo-mm-s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx AXI-Stream FIFO
> +
> +maintainers:
> +  - Jacob Feder <jacobsfeder@gmail.com>
> +
> +description:
> +  The Xilinx AXI-Stream FIFO (PG080) provides a memory-mapped interface to
> +  an AXI-Stream FIFO IP core. It allows a processor to transmit and receive
> +  AXI-Stream packets via simple MMIO register reads and writes.

Does the driver use the correct MMIO accessors for the FIFO data?

drivers/staging/axis-fifo/axis-fifo.c:axis_fifo_write() {
    ...
	for (int i = 0; i < words_to_write; ++i)
        // driver loops over the stream data offsets using iowrite32
    ...
}

According to the I/O Accessors subsystem guidelines, FIFOs and stream-oriented
hardware must exclusively use stream accessors (e.g., ioread32_rep(), readsl())
to preserve memory-to-device byte order.

Since the driver loops over the stream data offsets using ioread32() and
iowrite32(), won't these accessors enforce CPU-to-device endianness conversion
and cause data payloads to be incorrectly byte-swapped on big-endian
architectures?

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - xlnx,axi-fifo-mm-s-4.1
> +      - xlnx,axi-fifo-mm-s-4.2
> +      - xlnx,axi-fifo-mm-s-4.3
> +
> +  reg:
> +    maxItems: 1

Does the binding need a clocks property to avoid a regression on power
management?

The Xilinx AXI-Stream FIFO is an AXI peripheral that requires an active AXI
clock (e.g., s_axi_aclk) to perform register transactions.

In the driver, axis_fifo_probe() maps the registers and performs MMIO writes
without requesting or enabling a clock via clk_prepare_enable().

If the clock is disabled by the bootloader or a power management unit, won't
these register accesses cause a synchronous external abort and crash or hang
the system?

> +
> +  interrupts:
> +    maxItems: 1

Does the driver's interrupt handler have a regression where unhandled
interrupts are dropped?

drivers/staging/axis-fifo/axis-fifo.c:axis_fifo_irq() {
    ...
	ier = ioread32(fifo->base_addr + XLLF_IER_OFFSET);
	isr = ioread32(fifo->base_addr + XLLF_ISR_OFFSET);
	intr = ier & isr;
    ...
	iowrite32(XLLF_INT_CLEAR_ALL, fifo->base_addr + XLLF_ISR_OFFSET);

	return IRQ_HANDLED;
}

Because XLLF_ISR_OFFSET is a Write-1-to-Clear register, unconditionally writing
0xFFFFFFFF (XLLF_INT_CLEAR_ALL) acknowledges and clears all interrupts, including
any new ones that the hardware asserted between the read and the write.

If a TX or RX complete interrupt fires in this narrow window, won't it be cleared
without being handled, causing the wait_event_interruptible() threads to hang?

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - xlnx,axi-str-rxd-tdata-width
> +  - xlnx,axi-str-txd-tdata-width
> +  - xlnx,rx-fifo-depth
> +  - xlnx,tx-fifo-depth
> +  - xlnx,use-rx-data
> +  - xlnx,use-tx-data
> +
> +additionalProperties: false

Will this cause a regression for existing device trees?

The original text binding in drivers/staging/axis-fifo/axis-fifo.txt required
properties such as interrupt-names, xlnx,axi-str-rxd-protocol,
xlnx,has-axis-tkeep, and many others describing physical hardware parameters
generated by Xilinx synthesis tools.

By omitting these properties while setting additionalProperties to false, won't
this cause spurious dtbs_check validation errors for existing, correctly formed
device trees?

Additionally, since the legacy axis-fifo.txt file is not deleted in this
commit, does this create duplicate and conflicting sources of truth for the
hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509171648.46933-1-pramod.nexgen@gmail.com?part=1