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
>