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

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

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 <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..
> 
> 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