On Tue, Sep 16, 2025 at 09:30:46AM +0200, Luc Michel wrote:
> 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, ...).
Yes, and for example the dynamic machine creation that Mirela
prototyped. I don't feel very strongly about this and I'm fine either
way. We can change things if a dynamic machine implementation comes
along.
>
> [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."
Aha, thanks!
But then is there really a case when the dummy one is needed? won't
versal-virt always pass an fdt?
If that is the case then maybe we could just assert(s->cfg.fdt);
>
> 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.
Got it, thanks!
>
> Thanks
>
> --
> Luc
