Re: [PATCH v5 02/47] hw/arm/xlnx-versal: prepare for FDT creation

["Edgar E. Iglesias" <edgar.iglesias@amd.com>]

On Fri, Sep 12, 2025 at 12:00:11PM +0200, Luc Michel wrote:
> The following commits will move FDT creation logic from the
> xlnx-versal-virt machine to the xlnx-versal SoC itself. Prepare this by
> passing the FDT handle to the SoC before it is realized. If no FDT is
> passed, a dummy one is created internally as a stub to the fdt function
> calls.
> 
> For now the SoC only creates the two clock nodes. The ones from the
> xlnx-versal virt machine are renamed with a `old-' prefix and will be
> removed once they are not referenced anymore.


Hi Luc,



> 
> Signed-off-by: Luc Michel <luc.michel@amd.com>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
> ---
>  include/hw/arm/xlnx-versal.h | 12 ++++++++++++
>  hw/arm/xlnx-versal-virt.c    |  9 ++++++---
>  hw/arm/xlnx-versal.c         | 28 ++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index 1f92e314d6c..f2a62b43552 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -134,21 +134,33 @@ struct Versal {
>          XlnxVersalCFrameBcastReg cframe_bcast;
>  
>          OrIRQState apb_irq_orgate;
>      } pmc;
>  
> +    struct {
> +        uint32_t clk_25mhz;
> +        uint32_t clk_125mhz;
> +    } phandle;
> +
>      struct {
>          MemoryRegion *mr_ddr;
> +        void *fdt;
>      } cfg;
>  };
>  
>  struct VersalClass {
>      SysBusDeviceClass parent;
>  
>      VersalVersion version;
>  };
>  
> +static inline void versal_set_fdt(Versal *s, void *fdt)
> +{
> +    g_assert(!qdev_is_realized(DEVICE(s)));
> +    s->cfg.fdt = fdt;
> +}
> +

Should this be a property of some sort? it looks a little odd to bypass QOM..






>  /* Memory-map and IRQ definitions. Copied a subset from
>   * auto-generated files.  */
>  
>  #define VERSAL_GIC_MAINT_IRQ        9
>  #define VERSAL_TIMER_VIRT_IRQ       11
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index adadbb72902..d1c65afa2ac 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -1,9 +1,10 @@
>  /*
>   * Xilinx Versal Virtual board.
>   *
>   * Copyright (c) 2018 Xilinx Inc.
> + * Copyright (c) 2025 Advanced Micro Devices, Inc.
>   * Written by Edgar E. Iglesias
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 or
>   * (at your option) any later version.
> @@ -695,14 +696,16 @@ static void versal_virt_init(MachineState *machine)
>                               &error_abort);
>      object_property_set_link(OBJECT(&s->soc), "canbus0", OBJECT(s->canbus[0]),
>                               &error_abort);
>      object_property_set_link(OBJECT(&s->soc), "canbus1", OBJECT(s->canbus[1]),
>                               &error_abort);
> -    sysbus_realize(SYS_BUS_DEVICE(&s->soc), &error_fatal);
>  
>      fdt_create(s);
> +    versal_set_fdt(&s->soc, s->fdt);
> +    sysbus_realize(SYS_BUS_DEVICE(&s->soc), &error_fatal);
>      create_virtio_regions(s);
> +
>      fdt_add_gem_nodes(s);
>      fdt_add_uart_nodes(s);
>      fdt_add_canfd_nodes(s);
>      fdt_add_gic_nodes(s);
>      fdt_add_timer_nodes(s);
> @@ -712,12 +715,12 @@ static void versal_virt_init(MachineState *machine)
>      fdt_add_rtc_node(s);
>      fdt_add_bbram_node(s);
>      fdt_add_efuse_ctrl_node(s);
>      fdt_add_efuse_cache_node(s);
>      fdt_add_cpu_nodes(s, psci_conduit);
> -    fdt_add_clk_node(s, "/clk125", 125000000, s->phandle.clk_125Mhz);
> -    fdt_add_clk_node(s, "/clk25", 25000000, s->phandle.clk_25Mhz);
> +    fdt_add_clk_node(s, "/old-clk125", 125000000, s->phandle.clk_125Mhz);
> +    fdt_add_clk_node(s, "/old-clk25", 25000000, s->phandle.clk_25Mhz);
>  
>      /* Make the APU cpu address space visible to virtio and other
>       * modules unaware of multiple address-spaces.  */
>      memory_region_add_subregion_overlap(get_system_memory(),
>                                          0, &s->soc.fpd.apu.mr, 0);
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index 4da656318f6..fda8fdf786a 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -22,10 +22,12 @@
>  #include "hw/misc/unimp.h"
>  #include "hw/arm/xlnx-versal.h"
>  #include "qemu/log.h"
>  #include "target/arm/cpu-qom.h"
>  #include "target/arm/gtimer.h"
> +#include "system/device_tree.h"
> +#include "hw/arm/fdt.h"
>  
>  #define XLNX_VERSAL_ACPU_TYPE ARM_CPU_TYPE_NAME("cortex-a72")
>  #define XLNX_VERSAL_RCPU_TYPE ARM_CPU_TYPE_NAME("cortex-r5f")
>  #define GEM_REVISION        0x40070106
>  
> @@ -917,15 +919,41 @@ static void versal_unimp(Versal *s)
>      qdev_connect_gpio_out_named(DEVICE(&s->pmc.iou.slcr),
>                                  SYSBUS_DEVICE_GPIO_IRQ, 0,
>                                  gpio_in);
>  }
>  
> +static uint32_t fdt_add_clk_node(Versal *s, const char *name,
> +                                 unsigned int freq_hz)
> +{
> +    uint32_t phandle;
> +
> +    phandle = qemu_fdt_alloc_phandle(s->cfg.fdt);
> +
> +    qemu_fdt_add_subnode(s->cfg.fdt, name);
> +    qemu_fdt_setprop_cell(s->cfg.fdt, name, "phandle", phandle);
> +    qemu_fdt_setprop_cell(s->cfg.fdt, name, "clock-frequency", freq_hz);
> +    qemu_fdt_setprop_cell(s->cfg.fdt, name, "#clock-cells", 0x0);
> +    qemu_fdt_setprop_string(s->cfg.fdt, name, "compatible", "fixed-clock");
> +    qemu_fdt_setprop(s->cfg.fdt, name, "u-boot,dm-pre-reloc", NULL, 0);
> +
> +    return phandle;
> +}
> +
>  static void versal_realize(DeviceState *dev, Error **errp)
>  {
>      Versal *s = XLNX_VERSAL_BASE(dev);
>      qemu_irq pic[XLNX_VERSAL_NR_IRQS];
>  
> +    if (s->cfg.fdt == NULL) {
> +        int fdt_size;
> +
> +        s->cfg.fdt = create_device_tree(&fdt_size);
> +    }
> +
> +    s->phandle.clk_25mhz = fdt_add_clk_node(s, "/clk25", 25 * 1000 * 1000);
> +    s->phandle.clk_125mhz = fdt_add_clk_node(s, "/clk125", 125 * 1000 * 1000);
> +

Should we be adding nodes if s->cfg.fdt wasn't created by us?
If the user passes a dtb, I wonder if we should just assume the user
knows what they are doing and use it as is...

Or do you have use-cases where it makes sense?




>      versal_create_apu_cpus(s);
>      versal_create_apu_gic(s, pic);
>      versal_create_rpu_cpus(s);
>      versal_create_uarts(s, pic);
>      versal_create_canfds(s, pic);
> -- 
> 2.50.1
>