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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/05/2025 21:59, Maciej Andrzejewski wrote:
> This module implements a driver for the Xilinx AXI Timeout Block (ATB).
> 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.
> 
> To make this driver work, the device tree must contain the following
> properties:
> 
> 	atb {
> 		compatible = "xlnx,zynqmp-atb";
> 		status = "okay";
> 
> 		fpd_afifs1_enable;
> 		fpd_afifs0_enable;
> 		fpd_fpds_enable;
> 		lpd_afifs2_enable;
> 		lpd_lpdm_enable;
> 	};
> 
> 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.
> 
> Signed-off-by: Maciej Andrzejewski <maciej.andrzejewski@m-works.net>
> Signed-off-by: Maciej Andrzejewski <maciej.andrzejewski@iceye.com>
> ---
>  MAINTAINERS                     |   6 +
>  drivers/soc/xilinx/Kconfig      |  53 ++--
>  drivers/soc/xilinx/Makefile     |   1 +
>  drivers/soc/xilinx/zynqmp_atb.c | 417 ++++++++++++++++++++++++++++++++
>  4 files changed, 457 insertions(+), 20 deletions(-)
>  create mode 100644 drivers/soc/xilinx/zynqmp_atb.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3c6f9e70ace0..37cd15a59d83 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26734,6 +26734,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/nvmem/xlnx,zynqmp-nvmem.yaml
>  F:	drivers/nvmem/zynqmp_nvmem.c
>  
> +XILINX ZYNQMP ATB DRIVER
> +M:	Maciej Andrzejewski <maciej.andrzejewski@m-works.net>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/soc/xilinx/xlnx,zynqmp-atb.txt
> +F:	drivers/soc/xilinx/zynqmp_atb.c
> +
>  XILLYBUS DRIVER
>  M:	Eli Billauer <eli.billauer@gmail.com>
>  L:	linux-kernel@vger.kernel.org
> diff --git a/drivers/soc/xilinx/Kconfig b/drivers/soc/xilinx/Kconfig
> index 49d69d6e18fe..b2b1187c70ec 100644
> --- a/drivers/soc/xilinx/Kconfig
> +++ b/drivers/soc/xilinx/Kconfig
> @@ -2,28 +2,41 @@
>  menu "Xilinx SoC drivers"
>  
>  config ZYNQMP_POWER
> -	bool "Enable Xilinx Zynq MPSoC Power Management driver"
> -	depends on PM && ZYNQMP_FIRMWARE
> -	default y
> -	select MAILBOX
> -	select ZYNQMP_IPI_MBOX
> -	help
> -	  Say yes to enable power management support for ZyqnMP SoC.
> -	  This driver uses firmware driver as an interface for power
> -	  management request to firmware. It registers isr to handle
> -	  power management callbacks from firmware. It registers mailbox client
> -	  to handle power management callbacks from firmware.
> +    bool "Enable Xilinx Zynq MPSoC Power Management driver"
> +    depends on PM && ZYNQMP_FIRMWARE
> +    default y
> +    select MAILBOX
> +    select ZYNQMP_IPI_MBOX

Why doing this change? Code was correct before.

> +    help
> +      Say yes to enable power management support for ZyqnMP SoC.
> +      This driver uses firmware driver as an interface for power
> +      management request to firmware. It registers isr to handle
> +      power management callbacks from firmware. It registers mailbox client
> +      to handle power management callbacks from firmware.
>  
> -	  If in doubt, say N.
> +      If in doubt, say N.
>  
>  config XLNX_EVENT_MANAGER
> -	bool "Enable Xilinx Event Management Driver"
> -	depends on ZYNQMP_FIRMWARE
> -	default ZYNQMP_FIRMWARE
> -	help
> -	  Say yes to enable event management support for Xilinx.
> -	  This driver uses firmware driver as an interface for event/power
> -	  management request to firmware.
> +    bool "Enable Xilinx Event Management Driver"
> +    depends on ZYNQMP_FIRMWARE
> +    default ZYNQMP_FIRMWARE
> +    help
> +      Say yes to enable event management support for Xilinx.
> +      This driver uses firmware driver as an interface for event/power
> +      management request to firmware.
> +
> +      If in doubt, say N.
> +
> +config ZYNQMP_ATB
> +    bool "Enable ZynqMP AXI Timeout Block driver"
> +    depends on ARCH_ZYNQMP
> +    help
> +      Say yes to enable ZynqMP AXI Timeout Block driver.
> +      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.
>  
> -	  If in doubt, say N.
>  endmenu
> diff --git a/drivers/soc/xilinx/Makefile b/drivers/soc/xilinx/Makefile
> index 33d94395fd87..30768593701e 100644
> --- a/drivers/soc/xilinx/Makefile
> +++ b/drivers/soc/xilinx/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_ZYNQMP_POWER)	+= zynqmp_power.o
>  obj-$(CONFIG_XLNX_EVENT_MANAGER)	+= xlnx_event_manager.o
> +obj-$(CONFIG_ZYNQMP_ATB) += zynqmp_atb.o
> \ No newline at end of file

All youro patches have patch warnings.

> diff --git a/drivers/soc/xilinx/zynqmp_atb.c b/drivers/soc/xilinx/zynqmp_atb.c
> new file mode 100644
> index 000000000000..41e942352b38
> --- /dev/null
> +++ b/drivers/soc/xilinx/zynqmp_atb.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/**
> + * Implementation of Xilinx ZynqMP AXI Timeout Block peripheral driver.
> + *
> + * Copyright (C) 2025 ICEYE,
> + * Maciej Andrzejewski <maciej.andrzejewski@iceye.com>
> + */
> +

...

> +
> +	return ret;
> +}
> +
> +/**
> + * read_dts - Read DTS configuration for the driver
> + * @config: Pointer to driver configuration
> + * @return: 0 on success, otherwise error code
> + */
> +static int read_dts(struct device *dev, struct atb_config *config)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_node_by_name(NULL, MOD_NAME);

No, you cannot do such stuff.

Match properly your driver.

> +	if (!np) {
> +		dev_err(dev, "unable to find device tree node '%s'", MOD_NAME);

So you will print such error on every machine, x86, arm (other vendors,
like Qualcomm), riscv64?


No, drop entirely.

> +		return -ENODEV;
> +	}
> +
> +	// FPD domain
> +	config->fpd.opt_val = 0;
> +	if (of_property_read_bool(np, "fpd_afifs1_enable"))
> +		config->fpd.opt_val |= FPD_ATB_AFIFS1_MASK;
> +	if (of_property_read_bool(np, "fpd_afifs0_enable"))
> +		config->fpd.opt_val |= FPD_ATB_AFIFS0_MASK;
> +	if (of_property_read_bool(np, "fpd_fpds_enable"))
> +		config->fpd.opt_val |= FPD_ATB_FPDS_MASK;
> +

...

> +
> +/**
> + * atb_domain_setup - Setup ATB in a domain
> + * @cfg: Pointer to ATB domain configuration
> + * @pdev: Pointer to platform device
> + * @base: Base address of the ATB domain
> + * @irq: IRQ number
> + * @domain_name: Domain name
> + * @irq_name: IRQ name
> + * @return: 0 on success, otherwise error code
> + */
> +static int atb_domain_setup(struct atb_domain_config *cfg,
> +			    struct platform_device *pdev, uintptr_t base,
> +			    int irq, const char *domain_name,
> +			    const char *irq_name)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	// get physical address
> +	cfg->base = devm_ioremap(dev, base, SZ_64);

No, you do not have MMIO. Just look at the binding.

> +	if (!cfg->base) {
> +		dev_err(dev, "failed to map %s base address", domain_name);
> +		return -1;
> +	}
> +
> +	// map IRQ
> +	irq = map_irq(dev, irq);

Look at the binding: you do not have interrupts.

> +	if (irq <= 0) {
> +		dev_err(dev, "failed to map ATB %s IRQ", domain_name);
> +		return -2;
> +	}
> +	cfg->irq = irq;
> +
> +	// request IRQ
> +	if (devm_request_irq(dev, irq, irq_handler, IRQF_SHARED, irq_name,
> +			     pdev)) {
> +		dev_err(dev, "failed to request ATB %s IRQ.", domain_name);
> +		return -3;
> +	}
> +
> +	// disable ATB for configuration
> +	atb_write(cfg, 0x0, ATB_RESP_EN_REG);
> +
> +	// response type: ATB returns 'SLVERR' on timeout
> +	// which results in app 'bus fault'
> +	atb_write(cfg, cfg->opt_val, ATB_RESP_TYPE_REG);
> +	atb_write(cfg, cfg->opt_val, ATB_CMD_STORE_EN_REG);
> +
> +	// timeout prescaler: 16b - enable, 15:0b - prescaler value
> +	// based on 100 MHz clock
> +	atb_write(cfg, (ATB_PRESCALER_EN_MASK + ATB_PRESCALER_TIMEOUT_VAL),
> +		  ATB_PRESCALE_REG);
> +
> +	// clear interrupt flags
> +	atb_write(cfg, cfg->opt_val, ERR_ATB_ISR_REG);
> +
> +	// enable selected interrupts
> +	atb_write(cfg, cfg->opt_val, ERR_ATB_IER_REG);
> +
> +	// enable ATB
> +	atb_write(cfg, cfg->opt_val, ATB_RESP_EN_REG);
> +
> +	cfg->init = true;
> +
> +	return 0;
> +}
> +
> +/**
> + * atb_probe - Probe function for the driver
> + * @pdev: Pointer to platform device
> + * @return: 0 on success, otherwise error code
> + */
> +static int atb_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct atb_config *config;
> +	struct device *dev = &pdev->dev;
> +
> +	config = devm_kzalloc(dev, sizeof(*config), GFP_KERNEL);
> +	if (!config)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, config);
> +
> +	// read DTS config
> +	ret = read_dts(dev, config);
> +	if (ret) {
> +		dev_err(dev, "failed to read DTS: %d", ret);
> +		goto err;
> +	}
> +
> +	// configure FPD
> +	ret = atb_domain_setup(&config->fpd, pdev, FPD_SLCR_ATB_BASE_ADDR,
> +			       IRQ_ZYNQMP_GIC_ATB_FPD, "FPD", "atb_fpd_irq");
> +	if (ret) {
> +		dev_err(dev, "failed to setup FPD domain: %d", ret);
> +		goto err;
> +	} else {
> +		dev_info(dev, "ATB FPD configured");
> +	}
> +
> +	// configure LPD
> +	ret = atb_domain_setup(&config->lpd, pdev, LPD_SLCR_ATB_BASE_ADDR,
> +			       IRQ_ZYNQMP_GIC_ATB_LPD, "LPD", "atb_lpd_irq");
> +	if (ret) {
> +		dev_err(dev, "failed to setup LPD domain: %d", ret);
> +		goto err_fpd;
> +	} else {
> +		dev_info(dev, "ATB LPD configured");

Drop all of such, drivers should be silent on success. See Linux coding
style.

> +	}
> +
> +	return 0;
> +
> +err_fpd:
> +	if (config->fpd.init)
> +		atb_write(&config->fpd, 0x0, ATB_RESP_EN_REG);
> +err:
> +	return -EFAULT;
> +}
> +
> +/**
> + * atb_remove - Remove function for the driver
> + * @pdev: Pointer to platform device
> + * @return: 0 on success, otherwise error code
> + */
> +static int atb_remove(struct platform_device *pdev)
> +{
> +	int val, reg;
> +	struct device *dev = &pdev->dev;
> +	struct atb_config *config = platform_get_drvdata(pdev);
> +
> +	if (!config) {
> +		dev_err(dev, "config is NULL in remove");
> +		return -EFAULT;
> +	}
> +
> +	// disable ATB
> +	if (config->fpd.init) {
> +		val = config->fpd.opt_val;
> +		reg = atb_read(&config->fpd, ATB_RESP_EN_REG);
> +		val &= ~reg;
> +		atb_write(&config->fpd, val, ATB_RESP_EN_REG);
> +	}
> +	if (config->lpd.init) {
> +		val = config->lpd.opt_val;
> +		reg = atb_read(&config->lpd, ATB_RESP_EN_REG);
> +		val &= ~reg;
> +		atb_write(&config->lpd, val, ATB_RESP_EN_REG);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id atb_of_match[] = {
> +	{
> +		.compatible = "xlnx,zynqmp-atb",
> +	},
> +	{},
> +};
> +
> +static struct platform_driver atb_driver = {
> +	.probe = atb_probe,
> +	.remove = atb_remove,
> +	.driver = {
> +		.name = MOD_NAME,
> +		.of_match_table = atb_of_match,
> +	},
> +};
> +
> +static int __init atb_early_init(void)
> +{
> +	return platform_driver_register(&atb_driver);
> +}
> +
> +arch_initcall(atb_early_init);

Don't order initcalls. That's not arch. This is supposed to be driver.


Best regards,
Krzysztof