Re: [PATCH 2/2] DT bindings docs for Driver Xilinx ZynqMP AXI Timeout Block (ATB)

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/05/2025 22:00, Maciej Andrzejewski wrote:
> Add DT bindings docs for Driver Xilinx ZynqMP AXI Timeout Block (ATB)
> 
> Signed-off-by: Maciej Andrzejewski <maciej.andrzejewski@m-works.net>
> Signed-off-by: Maciej Andrzejewski <maciej.andrzejewski@iceye.com>

One SoB is enough, considering you sent it from the first one.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters


Please organize the patch documenting compatible (DT bindings) before
their user.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

> ---
>  .../bindings/soc/xilinx/xlnx,zynqmp-atb.yaml  | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/xilinx/xlnx,zynqmp-atb.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/xilinx/xlnx,zynqmp-atb.yaml b/Documentation/devicetree/bindings/soc/xilinx/xlnx,zynqmp-atb.yaml
> new file mode 100644
> index 000000000000..d92747fba947
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/xilinx/xlnx,zynqmp-atb.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/xilinx/xlnx,zynqmp-atb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx ZynqMP AXI Timeout Block peripheral
> +
> +maintainers:
> +  - Maciej Andrzejewski <maciej.andrzejewski@m-works.net>
> +
> +description: |+
> +

Drop blank line

> +  This module implements a driver for the Xilinx AXI Timeout Block (ATB).

Binding is about hardware, not driver. Please rephrase everything here
to describe hardware.

> +  The ATB is used to detect and handle timeouts on AXI transactions.
> +  It supports configuration and handling of timeout interrupts for both
> +  the Full Power Domain (FPD) and Low Power Domain (LPD) in Xilinx SoCs.
> +  The driver reads configuration from the device tree, sets up the necessary
> +  registers, and handles interrupts to report timeout events.
> +
> +  AXI timeouts can be harmful as they stall the bus and can potentially
> +  stall the entire Linux system. Due to hardware limitations, this driver
> +  cannot completely prevent bus stalls; only a limited number of AXI timeouts
> +  can be registered before the bus will eventually stall.
> +
> +  It is important to note that this driver should produce dmesg error output
> +  (grep for "atb: Timeout detected") to notify the user that the system
> +  configuration is not properly handled. Once an AXI timeout is detected,
> +  additional measures should be taken to address the issue.
> +
> +  The bool properties enable the corresponding interrupts for the
> +  FPD and LPD.
> +  The driver will read these properties and configure the ATB accordingly.
> +  If the bool property is not present, the functionality for this ATB will be
> +  disabled.
> +
> +  Enable this driver by selecting ZYNQMP_ATB in the kernel config.
> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-atb
> +
> +  fpd_afifs1_enable:

Neither this nor any further properties look like we expect. Please open
any other bindings or example-schema to see how this is done - missing
vendor prefix, following DTS coding style for naming.

> +    description: |
> +      ATB instance 5: FPD Main Switch to M_AXI_HPM1_FPD interface.
> +      This is used to detect timeouts on the M_AXI_HPM1_FPD.
> +    type: boolean
> +    default: false

No, there is no such option. You cannot assign there value.

> +
> +  fpd_afifs0_enable:
> +    description: |
> +      ATB instance 4: FPD Main Switch to M_AXI_HPM0_FPD interface.
> +      This is used to detect timeouts on the M_AXI_HPM0_FPD.

I don't understand need for these, but could be because of missing
hardware description.


> +    type: boolean
> +    default: false
> +
> +  fpd_fpds_enable:
> +    description: |
> +      ATB instance 3: FPD Main Switch to SIOU slaves.
> +      This is used to detect timeouts on the SIOU slaves.
> +    type: boolean
> +    default: false
> +
> +  lpd_afifs2_enable:
> +    description: |
> +      ATB instance 2: LPD Main Switch to M_AXI_HPM2_LPD interface.
> +      This is used to detect timeouts on the M_AXI_HPM2_LPD.
> +    type: boolean
> +    default: false
> +
> +  lpd_lpdm_enable:
> +    description: |
> +      ATB instance 1: LPD Main Switch to M_AXI_LPD interface.
> +      This is used to detect timeouts on the M_AXI_LPD.
> +    type: boolean
> +    default: false
> +

This wasn't ever tested... Also, missing required and additionalProperties.

> +examples:
> +  - |
> +
> +  atb {
> +      compatible = "xlnx,zynqmp-atb";
> +      status = "okay";

Drop. This binding looks very different than any other. Look at others
first - nothing uses status, format is different, you miss several
important pieces of binding.

> +
> +      fpd_afifs1_enable;
> +      fpd_afifs0_enable;
> +      fpd_fpds_enable;
> +      lpd_afifs2_enable;
> +      lpd_lpdm_enable;
> +    };
> \ No newline at end of file

Patch warnings.



Best regards,
Krzysztof