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