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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 8/2/22 18:24, BALATON Zoltan wrote:
> 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.


Thanks for the explanation!

I'm not sure if this is something worth documenting in this comment or not. I
guess it wouldn't hurt to mention "the memory controller can map RAM in different
sockets but we don't implement that" or something similar. Or just remove the
comment entirely.

Both works for me as long as we get rid of the 'fix this' comment. It implies
that we're doing something wrong on purpose, which doesn't seem to be the
case.


Thanks,


Daniel




> 
> Regards,
> BALATON Zoltan