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

[BALATON Zoltan]

On Tue, 2 Aug 2022, 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.

I'm not sure about it because I don't know 405 and only vaguely remember 
how this was on 440/460EX but I think it might be that the memory 
controller can be programmed to map RAM to different places but we don't 
fully emulate that nor the different chunks/DIMM sockets that could be 
possible and just map all system RAM to address 0 which is what most guest 
firmware or OS does anyway. Maybe I'm wrong and don't remember this 
correctly, the SDRAM controller model in ppc4xx_devs.c seems to do some 
mapping but I think this is what the comment might refer to or something 
similar. If so, I don't think it's worth emulating this more precisely 
unless we know about a guest which needs this.

Regards,
BALATON Zoltan