Re: [PATCH 05/19] ppc/ppc405: Start QOMification of the SoC

[Cédric Le Goater]

On 8/3/22 13:59, BALATON Zoltan wrote:
> On Wed, 3 Aug 2022, Cédric Le Goater wrote:
> > On 8/2/22 21:18, Daniel Henrique Barboza wrote:
> > > On 8/1/22 10:10, Cédric Le Goater wrote:
> > > > This moves all the code previously done in the ppc405ep_init() routine
> > > > under ppc405_soc_realize().
> > > >
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > >   hw/ppc/ppc405.h        |  12 ++--
> > > >   hw/ppc/ppc405_boards.c |  12 ++--
> > > >   hw/ppc/ppc405_uc.c     | 151 ++++++++++++++++++++---------------------
> > > >   3 files changed, 84 insertions(+), 91 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> > > > index c8cddb71733a..5e4e96d86ceb 100644
> > > > --- a/hw/ppc/ppc405.h
> > > > +++ b/hw/ppc/ppc405.h
> > > > @@ -74,9 +74,14 @@ struct Ppc405SoCState {
> > > >       MemoryRegion sram;
> > > >       MemoryRegion ram_memories[2];
> > > >       hwaddr ram_bases[2], ram_sizes[2];
> > > > +    bool do_dram_init;
> > > >       MemoryRegion *dram_mr;
> > > >       hwaddr ram_size;
> > > > +
> > > > +    uint32_t sysclk;
> > > > +    PowerPCCPU *cpu;
> > > > +    DeviceState *uic;
> > > >   };
> > > >   /* PowerPC 405 core */
> > > > @@ -85,11 +90,4 @@ ram_addr_t ppc405_set_bootinfo(CPUPPCState *env, ram_addr_t 
> > > > ram_size);
> > > >   void ppc4xx_plb_init(CPUPPCState *env);
> > > >   void ppc405_ebc_init(CPUPPCState *env);
> > > > -PowerPCCPU *ppc405ep_init(MemoryRegion *address_space_mem,
> > > > -                        MemoryRegion ram_memories[2],
> > > > -                        hwaddr ram_bases[2],
> > > > -                        hwaddr ram_sizes[2],
> > > > -                        uint32_t sysclk, DeviceState **uicdev,
> > > > -                        int do_init);
> > > > -
> > > >   #endif /* PPC405_H */
> > > > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> > > > index 96db52c5a309..363cb0770506 100644
> > > > --- a/hw/ppc/ppc405_boards.c
> > > > +++ b/hw/ppc/ppc405_boards.c
> > > > @@ -237,9 +237,7 @@ static void ppc405_init(MachineState *machine)
> > > >       Ppc405MachineState *ppc405 = PPC405_MACHINE(machine);
> > > >       MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > >       const char *kernel_filename = machine->kernel_filename;
> > > > -    PowerPCCPU *cpu;
> > > >       MemoryRegion *sysmem = get_system_memory();
> > > > -    DeviceState *uicdev;
> > > >       if (machine->ram_size != mc->default_ram_size) {
> > > >           char *sz = size_to_str(mc->default_ram_size);
> > > > @@ -254,12 +252,12 @@ static void ppc405_init(MachineState *machine)
> > > >                                machine->ram_size, &error_fatal);
> > > >       object_property_set_link(OBJECT(&ppc405->soc), "dram",
> > > >                                OBJECT(machine->ram), &error_abort);
> > > > +    object_property_set_bool(OBJECT(&ppc405->soc), "dram-init",
> > > > +                             !(kernel_filename == NULL), &error_abort);
> > > > +    object_property_set_uint(OBJECT(&ppc405->soc), "sys-clk", 33333333,
> > > > +                             &error_abort);
> > > >       qdev_realize(DEVICE(&ppc405->soc), NULL, &error_abort);
> > > > -    cpu = ppc405ep_init(sysmem, ppc405->soc.ram_memories, 
> > > > ppc405->soc.ram_bases,
> > > > -                        ppc405->soc.ram_sizes,
> > > > -                        33333333, &uicdev, kernel_filename == NULL ? 0 : 1);
> > > > -
> > > >       /* allocate and load BIOS */
> > > >       if (machine->firmware) {
> > > >           MemoryRegion *bios = g_new(MemoryRegion, 1);
> > > > @@ -315,7 +313,7 @@ static void ppc405_init(MachineState *machine)
> > > >       /* Load ELF kernel and rootfs.cpio */
> > > >       } else if (kernel_filename && !machine->firmware) {
> > > > -        boot_from_kernel(machine, cpu);
> > > > +        boot_from_kernel(machine, ppc405->soc.cpu);
> > > >       }
> > > >   }
> > > > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> > > > index 156e839b8283..59612504bf3f 100644
> > > > --- a/hw/ppc/ppc405_uc.c
> > > > +++ b/hw/ppc/ppc405_uc.c
> > > > @@ -1432,134 +1432,131 @@ static void ppc405ep_cpc_init (CPUPPCState *env, 
> > > > clk_setup_t clk_setup[8],
> > > >   #endif
> > > >   }
> > > > -PowerPCCPU *ppc405ep_init(MemoryRegion *address_space_mem,
> > > > -                        MemoryRegion ram_memories[2],
> > > > -                        hwaddr ram_bases[2],
> > > > -                        hwaddr ram_sizes[2],
> > > > -                        uint32_t sysclk, DeviceState **uicdevp,
> > > > -                        int do_init)
> > > > +static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> > > >   {
> > > > +    Ppc405SoCState *s = PPC405_SOC(dev);
> > > >       clk_setup_t clk_setup[PPC405EP_CLK_NB], tlb_clk_setup;
> > > >       qemu_irq dma_irqs[4], gpt_irqs[5], mal_irqs[4];
> > > > -    PowerPCCPU *cpu;
> > > >       CPUPPCState *env;
> > > > -    DeviceState *uicdev;
> > > > -    SysBusDevice *uicsbd;
> > > > +    Error *err = NULL;
> > > > +
> > > > +    /* XXX: fix this ? */
> > >
> > > So, this comment, originally from ppc405_boards.c, was added by commit
> > > 1a6c088620368 and it seemed to make reference to something with the refering
> > > to the ram_* values:
> > >
> > >
> > >      /* XXX: fix this */
> > >      ram_bases[0] = 0x00000000;
> > >      ram_sizes[0] = 0x08000000;
> > >      ram_bases[1] = 0x00000000;
> > >      ram_sizes[1] = 0x00000000;
> > > (...)
> > >
> > >
> > > No more context is provided aside from a git-svn-id from savannah.nongnu.org.
> > >
> > > If no one can provide more context about what is to be fixed here, I'll
> > > remove the comment.
> > >
> > >
> > >
> > > > +    memory_region_init_alias(&s->ram_memories[0], OBJECT(s),
> > > > +                             "ef405ep.ram.alias", s->dram_mr, 0, s->ram_size);
> > >
> > > As I mentioned in patch 2, ef405ep.ram.alias can be renamed to ppc405.ram.alias 
> > > ...
> >
> > sure.
> >
> > > > +    s->ram_bases[0] = 0;
> > > > +    s->ram_sizes[0] = s->ram_size;
> > > > +    memory_region_init(&s->ram_memories[1], OBJECT(s), "ef405ep.ram1", 0);
> > >
> > > And this can be renamed to ef405ep.ram1. If you agree with the rename I
> > > can amend it in the tree.
> >
> >
> > I think we can do better and simply remove the second bank. it is unused ...
> >
> > I have patches QOMifying ppc4xx_sdram_init() but the current modelling
> > makes things a bit complex, specially ppc4xx_sdram_banks() which is only
> > used on the bamboo and sam460ex machines.
>
> We need to model the SDRAM controller for the sam460ex firmware at least partially so it gets past the memory test and init. If you change it please test that sam460ex still boots with firmware. 

Sure.

QOM'ifying models reveals the implementation shortcuts. The ppc405 board was
quite messy and it's getting better with the removal of ppc405ep_init().
ppc4xx_sdram_init() is the next step but it is quite localized in the SoC.
No hurries.

Thanks,

C.


> I'm not sure 405 and 440 use the same sdram controller though or if the 460EX 
> has a different one as these may have been updated in later SoCs to support 
> newer RAM standards (I think the 460EX has DDR2) and the QEMU model is a bit 
> messy sharing components between 405 and 440. When I've changed some of these 
> for 460EX I've tried to name them so that 4xx means shared by all, 44x or 440 
> means 440 specific and 40x or similar for older SoCs only but this is probably 
> not always correct. I could not find a clean way to separate these. QOMifying 
> would make sense when cleaning this up otherwise it's just adding more 
> boilerplate without any clear advantage IMO.
>
> If you want change these maybe you should get the docs for the emulated chips 
> and consult those for differences. Unfortunately the 460ex does not seem to 
> have docs available but it's similar to earlier IBM parts like 440GP which 
> generally have docs. That's what I've used but still couldn't find all parts 
> like the PCIe conroller that I had to implement based on what the firmware and 
> drivers do (and only did that partially so it boots as I did not fully 
> understand how it works and how these are modelled in QEMU).
>
> Regards,
> BALATON Zoltan