Hi Edgar,
On 19:17 Fri 12 Sep , Edgar E. Iglesias wrote:
> 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 <[email protected]>
> > Reviewed-by: Francisco Iglesias <[email protected]>
> > ---
> > 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..
fdt being a void* and not an Object*, it's not directly possible AFAIK.
I don't see it being an issue here because the Versal SoC code is
tightly coupled to the versal-virt machine code (the machine is
basically the sole user of the SoC). Even if it was not the case, the
SoC interface is fully specified in xlnx-versal.h and any user can
leverage it just fine. I guess QOM/qdev abstractions are necessary when
we don't include the .h and only rely on the type name (QMP, HPM
use-cases, ...).
[snip]
> > +
> > 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?
Note that the device tree created in the SoC code is just a dummy one to
avoid crashing when the SoC user does not provide one, as stated in the
commit message:
"If no FDT is passed, a dummy one is created internally as a stub to the
fdt function calls."
This code path should not be reached in normal versal-virt machine
use-case. We rely on the one given by the machine code through the
versal_set_fdt function.
Then to answer the question about the user providing a DTB, I stick to
the existing behaviour before the refactoring. s->binfo.get_dtb is still
set in the machine code and provided to the ARM virtual bootloader. The
bootloader uses it only if no DTB is provided by the user.
Thanks
--
Luc
