diff mbox series

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

Message ID 20220801131039.1693913-6-clg@kaod.org (mailing list archive)
State New, archived
Headers show
Series ppc: QOM'ify 405 board | expand

Commit Message

Cédric Le Goater Aug. 1, 2022, 1:10 p.m. UTC
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(-)

Comments

Daniel Henrique Barboza Aug. 2, 2022, 7:18 p.m. UTC | #1
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 ...

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



Thanks,


Daniel

> +    s->ram_bases[1] = 0x00000000;
> +    s->ram_sizes[1] = 0x00000000;
> +
> +    /* allocate SRAM */
> +    memory_region_init_ram(&s->sram, OBJECT(s), "ef405ep.sram",
> +                           PPC405EP_SRAM_SIZE, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    memory_region_add_subregion(get_system_memory(), PPC405EP_SRAM_BASE,
> +                                &s->sram);
>   
>       memset(clk_setup, 0, sizeof(clk_setup));
> +
>       /* init CPUs */
> -    cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"),
> +    s->cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"),
>                         &clk_setup[PPC405EP_CPU_CLK],
> -                      &tlb_clk_setup, sysclk);
> -    env = &cpu->env;
> +                      &tlb_clk_setup, s->sysclk);
> +    env = &s->cpu->env;
>       clk_setup[PPC405EP_CPU_CLK].cb = tlb_clk_setup.cb;
>       clk_setup[PPC405EP_CPU_CLK].opaque = tlb_clk_setup.opaque;
> -    /* Internal devices init */
> -    /* Memory mapped devices registers */
> +
> +    /* CPU control */
> +    ppc405ep_cpc_init(env, clk_setup, s->sysclk);
> +
>       /* PLB arbitrer */
>       ppc4xx_plb_init(env);
> +
>       /* PLB to OPB bridge */
>       ppc4xx_pob_init(env);
> +
>       /* OBP arbitrer */
>       ppc4xx_opba_init(0xef600600);
> +
>       /* Universal interrupt controller */
> -    uicdev = qdev_new(TYPE_PPC_UIC);
> -    uicsbd = SYS_BUS_DEVICE(uicdev);
> +    s->uic = qdev_new(TYPE_PPC_UIC);
>   
> -    object_property_set_link(OBJECT(uicdev), "cpu", OBJECT(cpu),
> +    object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(s->cpu),
>                                &error_fatal);
> -    sysbus_realize_and_unref(uicsbd, &error_fatal);
> -
> -    sysbus_connect_irq(uicsbd, PPCUIC_OUTPUT_INT,
> -                       qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT));
> -    sysbus_connect_irq(uicsbd, PPCUIC_OUTPUT_CINT,
> -                       qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_CINT));
> +    if (!sysbus_realize(SYS_BUS_DEVICE(s->uic), errp)) {
> +        return;
> +    }
>   
> -    *uicdevp = uicdev;
> +    sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_INT,
> +                       qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_INT));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_CINT,
> +                       qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_CINT));
>   
>       /* SDRAM controller */
> -        /* XXX 405EP has no ECC interrupt */
> -    ppc4xx_sdram_init(env, qdev_get_gpio_in(uicdev, 17), 2, ram_memories,
> -                      ram_bases, ram_sizes, do_init);
> +    /* XXX 405EP has no ECC interrupt */
> +    ppc4xx_sdram_init(env, qdev_get_gpio_in(s->uic, 17), 2, s->ram_memories,
> +                      s->ram_bases, s->ram_sizes, s->do_dram_init);
> +
>       /* External bus controller */
>       ppc405_ebc_init(env);
> +
>       /* DMA controller */
> -    dma_irqs[0] = qdev_get_gpio_in(uicdev, 5);
> -    dma_irqs[1] = qdev_get_gpio_in(uicdev, 6);
> -    dma_irqs[2] = qdev_get_gpio_in(uicdev, 7);
> -    dma_irqs[3] = qdev_get_gpio_in(uicdev, 8);
> +    dma_irqs[0] = qdev_get_gpio_in(s->uic, 5);
> +    dma_irqs[1] = qdev_get_gpio_in(s->uic, 6);
> +    dma_irqs[2] = qdev_get_gpio_in(s->uic, 7);
> +    dma_irqs[3] = qdev_get_gpio_in(s->uic, 8);
>       ppc405_dma_init(env, dma_irqs);
> -    /* IIC controller */
> +
> +    /* I2C controller */
>       sysbus_create_simple(TYPE_PPC4xx_I2C, 0xef600500,
> -                         qdev_get_gpio_in(uicdev, 2));
> +                         qdev_get_gpio_in(s->uic, 2));
>       /* GPIO */
>       ppc405_gpio_init(0xef600700);
> +
>       /* Serial ports */
>       if (serial_hd(0) != NULL) {
> -        serial_mm_init(address_space_mem, 0xef600300, 0,
> -                       qdev_get_gpio_in(uicdev, 0),
> +        serial_mm_init(get_system_memory(), 0xef600300, 0,
> +                       qdev_get_gpio_in(s->uic, 0),
>                          PPC_SERIAL_MM_BAUDBASE, serial_hd(0),
>                          DEVICE_BIG_ENDIAN);
>       }
>       if (serial_hd(1) != NULL) {
> -        serial_mm_init(address_space_mem, 0xef600400, 0,
> -                       qdev_get_gpio_in(uicdev, 1),
> +        serial_mm_init(get_system_memory(), 0xef600400, 0,
> +                       qdev_get_gpio_in(s->uic, 1),
>                          PPC_SERIAL_MM_BAUDBASE, serial_hd(1),
>                          DEVICE_BIG_ENDIAN);
>       }
> +
>       /* OCM */
>       ppc405_ocm_init(env);
> +
>       /* GPT */
> -    gpt_irqs[0] = qdev_get_gpio_in(uicdev, 19);
> -    gpt_irqs[1] = qdev_get_gpio_in(uicdev, 20);
> -    gpt_irqs[2] = qdev_get_gpio_in(uicdev, 21);
> -    gpt_irqs[3] = qdev_get_gpio_in(uicdev, 22);
> -    gpt_irqs[4] = qdev_get_gpio_in(uicdev, 23);
> +    gpt_irqs[0] = qdev_get_gpio_in(s->uic, 19);
> +    gpt_irqs[1] = qdev_get_gpio_in(s->uic, 20);
> +    gpt_irqs[2] = qdev_get_gpio_in(s->uic, 21);
> +    gpt_irqs[3] = qdev_get_gpio_in(s->uic, 22);
> +    gpt_irqs[4] = qdev_get_gpio_in(s->uic, 23);
>       ppc4xx_gpt_init(0xef600000, gpt_irqs);
> -    /* PCI */
> -    /* Uses UIC IRQs 3, 16, 18 */
> +
>       /* MAL */
> -    mal_irqs[0] = qdev_get_gpio_in(uicdev, 11);
> -    mal_irqs[1] = qdev_get_gpio_in(uicdev, 12);
> -    mal_irqs[2] = qdev_get_gpio_in(uicdev, 13);
> -    mal_irqs[3] = qdev_get_gpio_in(uicdev, 14);
> +    mal_irqs[0] = qdev_get_gpio_in(s->uic, 11);
> +    mal_irqs[1] = qdev_get_gpio_in(s->uic, 12);
> +    mal_irqs[2] = qdev_get_gpio_in(s->uic, 13);
> +    mal_irqs[3] = qdev_get_gpio_in(s->uic, 14);
>       ppc4xx_mal_init(env, 4, 2, mal_irqs);
> +
>       /* Ethernet */
>       /* Uses UIC IRQs 9, 15, 17 */
> -    /* CPU control */
> -    ppc405ep_cpc_init(env, clk_setup, sysclk);
> -
> -    return cpu;
> -}
> -
> -static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> -{
> -    Ppc405SoCState *s = PPC405_SOC(dev);
> -    Error *err = NULL;
> -
> -    /* XXX: fix this ? */
> -    memory_region_init_alias(&s->ram_memories[0], OBJECT(s),
> -                             "ef405ep.ram.alias", s->dram_mr, 0, s->ram_size);
> -    s->ram_bases[0] = 0;
> -    s->ram_sizes[0] = s->ram_size;
> -    memory_region_init(&s->ram_memories[1], OBJECT(s), "ef405ep.ram1", 0);
> -    s->ram_bases[1] = 0x00000000;
> -    s->ram_sizes[1] = 0x00000000;
> -
> -    /* allocate SRAM */
> -    memory_region_init_ram(&s->sram, OBJECT(s), "ef405ep.sram",
> -                           PPC405EP_SRAM_SIZE,  &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        return;
> -    }
> -    memory_region_add_subregion(get_system_memory(), PPC405EP_SRAM_BASE,
> -                                &s->sram);
>   }
>   
>   static Property ppc405_soc_properties[] = {
>       DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
>                        MemoryRegion *),
> +    DEFINE_PROP_UINT32("sys-clk", Ppc405SoCState, sysclk, 0),
> +    DEFINE_PROP_BOOL("dram-init", Ppc405SoCState, do_dram_init, 0),
>       DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };
BALATON Zoltan Aug. 2, 2022, 9:24 p.m. UTC | #2
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
Cédric Le Goater Aug. 3, 2022, 7:54 a.m. UTC | #3
On 8/2/22 23: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 

Yes. See :

   https://gitlab.com/huth/u-boot/-/blob/taihu/arch/powerpc/cpu/ppc4xx/sdram.c

Thomas had to hack the SDRAM init sequence for the QEMU machine to boot.

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

It feels like it. At least for the 405, only one bank is used :

   0000000000000000-ffffffffffffffff (prio 0, i/o): system
     0000000000000000-0000000007ffffff (prio 0, i/o): sdram-containers
       0000000000000000-0000000007ffffff (prio 0, ram): alias ef405ep.ram.alias @ppc405.ram 0000000000000000-0000000007ffffff

The machine allocates a set of memory regions which are aliases on the
machine RAM. These are passed to the SDRAM controller model which maps
and unmaps container regions depending on the register settings.

It could be simplified.

Thanks,

C.


> 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
Cédric Le Goater Aug. 3, 2022, 8:03 a.m. UTC | #4
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.



C.


> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
>> +    s->ram_bases[1] = 0x00000000;
>> +    s->ram_sizes[1] = 0x00000000;
>> +
>> +    /* allocate SRAM */
>> +    memory_region_init_ram(&s->sram, OBJECT(s), "ef405ep.sram",
>> +                           PPC405EP_SRAM_SIZE, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +    memory_region_add_subregion(get_system_memory(), PPC405EP_SRAM_BASE,
>> +                                &s->sram);
>>       memset(clk_setup, 0, sizeof(clk_setup));
>> +
>>       /* init CPUs */
>> -    cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"),
>> +    s->cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"),
>>                         &clk_setup[PPC405EP_CPU_CLK],
>> -                      &tlb_clk_setup, sysclk);
>> -    env = &cpu->env;
>> +                      &tlb_clk_setup, s->sysclk);
>> +    env = &s->cpu->env;
>>       clk_setup[PPC405EP_CPU_CLK].cb = tlb_clk_setup.cb;
>>       clk_setup[PPC405EP_CPU_CLK].opaque = tlb_clk_setup.opaque;
>> -    /* Internal devices init */
>> -    /* Memory mapped devices registers */
>> +
>> +    /* CPU control */
>> +    ppc405ep_cpc_init(env, clk_setup, s->sysclk);
>> +
>>       /* PLB arbitrer */
>>       ppc4xx_plb_init(env);
>> +
>>       /* PLB to OPB bridge */
>>       ppc4xx_pob_init(env);
>> +
>>       /* OBP arbitrer */
>>       ppc4xx_opba_init(0xef600600);
>> +
>>       /* Universal interrupt controller */
>> -    uicdev = qdev_new(TYPE_PPC_UIC);
>> -    uicsbd = SYS_BUS_DEVICE(uicdev);
>> +    s->uic = qdev_new(TYPE_PPC_UIC);
>> -    object_property_set_link(OBJECT(uicdev), "cpu", OBJECT(cpu),
>> +    object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(s->cpu),
>>                                &error_fatal);
>> -    sysbus_realize_and_unref(uicsbd, &error_fatal);
>> -
>> -    sysbus_connect_irq(uicsbd, PPCUIC_OUTPUT_INT,
>> -                       qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT));
>> -    sysbus_connect_irq(uicsbd, PPCUIC_OUTPUT_CINT,
>> -                       qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_CINT));
>> +    if (!sysbus_realize(SYS_BUS_DEVICE(s->uic), errp)) {
>> +        return;
>> +    }
>> -    *uicdevp = uicdev;
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_INT,
>> +                       qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_INT));
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_CINT,
>> +                       qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_CINT));
>>       /* SDRAM controller */
>> -        /* XXX 405EP has no ECC interrupt */
>> -    ppc4xx_sdram_init(env, qdev_get_gpio_in(uicdev, 17), 2, ram_memories,
>> -                      ram_bases, ram_sizes, do_init);
>> +    /* XXX 405EP has no ECC interrupt */
>> +    ppc4xx_sdram_init(env, qdev_get_gpio_in(s->uic, 17), 2, s->ram_memories,
>> +                      s->ram_bases, s->ram_sizes, s->do_dram_init);
>> +
>>       /* External bus controller */
>>       ppc405_ebc_init(env);
>> +
>>       /* DMA controller */
>> -    dma_irqs[0] = qdev_get_gpio_in(uicdev, 5);
>> -    dma_irqs[1] = qdev_get_gpio_in(uicdev, 6);
>> -    dma_irqs[2] = qdev_get_gpio_in(uicdev, 7);
>> -    dma_irqs[3] = qdev_get_gpio_in(uicdev, 8);
>> +    dma_irqs[0] = qdev_get_gpio_in(s->uic, 5);
>> +    dma_irqs[1] = qdev_get_gpio_in(s->uic, 6);
>> +    dma_irqs[2] = qdev_get_gpio_in(s->uic, 7);
>> +    dma_irqs[3] = qdev_get_gpio_in(s->uic, 8);
>>       ppc405_dma_init(env, dma_irqs);
>> -    /* IIC controller */
>> +
>> +    /* I2C controller */
>>       sysbus_create_simple(TYPE_PPC4xx_I2C, 0xef600500,
>> -                         qdev_get_gpio_in(uicdev, 2));
>> +                         qdev_get_gpio_in(s->uic, 2));
>>       /* GPIO */
>>       ppc405_gpio_init(0xef600700);
>> +
>>       /* Serial ports */
>>       if (serial_hd(0) != NULL) {
>> -        serial_mm_init(address_space_mem, 0xef600300, 0,
>> -                       qdev_get_gpio_in(uicdev, 0),
>> +        serial_mm_init(get_system_memory(), 0xef600300, 0,
>> +                       qdev_get_gpio_in(s->uic, 0),
>>                          PPC_SERIAL_MM_BAUDBASE, serial_hd(0),
>>                          DEVICE_BIG_ENDIAN);
>>       }
>>       if (serial_hd(1) != NULL) {
>> -        serial_mm_init(address_space_mem, 0xef600400, 0,
>> -                       qdev_get_gpio_in(uicdev, 1),
>> +        serial_mm_init(get_system_memory(), 0xef600400, 0,
>> +                       qdev_get_gpio_in(s->uic, 1),
>>                          PPC_SERIAL_MM_BAUDBASE, serial_hd(1),
>>                          DEVICE_BIG_ENDIAN);
>>       }
>> +
>>       /* OCM */
>>       ppc405_ocm_init(env);
>> +
>>       /* GPT */
>> -    gpt_irqs[0] = qdev_get_gpio_in(uicdev, 19);
>> -    gpt_irqs[1] = qdev_get_gpio_in(uicdev, 20);
>> -    gpt_irqs[2] = qdev_get_gpio_in(uicdev, 21);
>> -    gpt_irqs[3] = qdev_get_gpio_in(uicdev, 22);
>> -    gpt_irqs[4] = qdev_get_gpio_in(uicdev, 23);
>> +    gpt_irqs[0] = qdev_get_gpio_in(s->uic, 19);
>> +    gpt_irqs[1] = qdev_get_gpio_in(s->uic, 20);
>> +    gpt_irqs[2] = qdev_get_gpio_in(s->uic, 21);
>> +    gpt_irqs[3] = qdev_get_gpio_in(s->uic, 22);
>> +    gpt_irqs[4] = qdev_get_gpio_in(s->uic, 23);
>>       ppc4xx_gpt_init(0xef600000, gpt_irqs);
>> -    /* PCI */
>> -    /* Uses UIC IRQs 3, 16, 18 */
>> +
>>       /* MAL */
>> -    mal_irqs[0] = qdev_get_gpio_in(uicdev, 11);
>> -    mal_irqs[1] = qdev_get_gpio_in(uicdev, 12);
>> -    mal_irqs[2] = qdev_get_gpio_in(uicdev, 13);
>> -    mal_irqs[3] = qdev_get_gpio_in(uicdev, 14);
>> +    mal_irqs[0] = qdev_get_gpio_in(s->uic, 11);
>> +    mal_irqs[1] = qdev_get_gpio_in(s->uic, 12);
>> +    mal_irqs[2] = qdev_get_gpio_in(s->uic, 13);
>> +    mal_irqs[3] = qdev_get_gpio_in(s->uic, 14);
>>       ppc4xx_mal_init(env, 4, 2, mal_irqs);
>> +
>>       /* Ethernet */
>>       /* Uses UIC IRQs 9, 15, 17 */
>> -    /* CPU control */
>> -    ppc405ep_cpc_init(env, clk_setup, sysclk);
>> -
>> -    return cpu;
>> -}
>> -
>> -static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>> -{
>> -    Ppc405SoCState *s = PPC405_SOC(dev);
>> -    Error *err = NULL;
>> -
>> -    /* XXX: fix this ? */
>> -    memory_region_init_alias(&s->ram_memories[0], OBJECT(s),
>> -                             "ef405ep.ram.alias", s->dram_mr, 0, s->ram_size);
>> -    s->ram_bases[0] = 0;
>> -    s->ram_sizes[0] = s->ram_size;
>> -    memory_region_init(&s->ram_memories[1], OBJECT(s), "ef405ep.ram1", 0);
>> -    s->ram_bases[1] = 0x00000000;
>> -    s->ram_sizes[1] = 0x00000000;
>> -
>> -    /* allocate SRAM */
>> -    memory_region_init_ram(&s->sram, OBJECT(s), "ef405ep.sram",
>> -                           PPC405EP_SRAM_SIZE,  &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> -        return;
>> -    }
>> -    memory_region_add_subregion(get_system_memory(), PPC405EP_SRAM_BASE,
>> -                                &s->sram);
>>   }
>>   static Property ppc405_soc_properties[] = {
>>       DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
>>                        MemoryRegion *),
>> +    DEFINE_PROP_UINT32("sys-clk", Ppc405SoCState, sysclk, 0),
>> +    DEFINE_PROP_BOOL("dram-init", Ppc405SoCState, do_dram_init, 0),
>>       DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
Daniel Henrique Barboza Aug. 3, 2022, 9:23 a.m. UTC | #5
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
BALATON Zoltan Aug. 3, 2022, 11:59 a.m. UTC | #6
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. 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
Cédric Le Goater Aug. 3, 2022, 12:28 p.m. UTC | #7
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
diff mbox series

Patch

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 ? */
+    memory_region_init_alias(&s->ram_memories[0], OBJECT(s),
+                             "ef405ep.ram.alias", s->dram_mr, 0, s->ram_size);
+    s->ram_bases[0] = 0;
+    s->ram_sizes[0] = s->ram_size;
+    memory_region_init(&s->ram_memories[1], OBJECT(s), "ef405ep.ram1", 0);
+    s->ram_bases[1] = 0x00000000;
+    s->ram_sizes[1] = 0x00000000;
+
+    /* allocate SRAM */
+    memory_region_init_ram(&s->sram, OBJECT(s), "ef405ep.sram",
+                           PPC405EP_SRAM_SIZE, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    memory_region_add_subregion(get_system_memory(), PPC405EP_SRAM_BASE,
+                                &s->sram);
 
     memset(clk_setup, 0, sizeof(clk_setup));
+
     /* init CPUs */
-    cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"),
+    s->cpu = ppc4xx_init(POWERPC_CPU_TYPE_NAME("405ep"),
                       &clk_setup[PPC405EP_CPU_CLK],
-                      &tlb_clk_setup, sysclk);
-    env = &cpu->env;
+                      &tlb_clk_setup, s->sysclk);
+    env = &s->cpu->env;
     clk_setup[PPC405EP_CPU_CLK].cb = tlb_clk_setup.cb;
     clk_setup[PPC405EP_CPU_CLK].opaque = tlb_clk_setup.opaque;
-    /* Internal devices init */
-    /* Memory mapped devices registers */
+
+    /* CPU control */
+    ppc405ep_cpc_init(env, clk_setup, s->sysclk);
+
     /* PLB arbitrer */
     ppc4xx_plb_init(env);
+
     /* PLB to OPB bridge */
     ppc4xx_pob_init(env);
+
     /* OBP arbitrer */
     ppc4xx_opba_init(0xef600600);
+
     /* Universal interrupt controller */
-    uicdev = qdev_new(TYPE_PPC_UIC);
-    uicsbd = SYS_BUS_DEVICE(uicdev);
+    s->uic = qdev_new(TYPE_PPC_UIC);
 
-    object_property_set_link(OBJECT(uicdev), "cpu", OBJECT(cpu),
+    object_property_set_link(OBJECT(s->uic), "cpu", OBJECT(s->cpu),
                              &error_fatal);
-    sysbus_realize_and_unref(uicsbd, &error_fatal);
-
-    sysbus_connect_irq(uicsbd, PPCUIC_OUTPUT_INT,
-                       qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT));
-    sysbus_connect_irq(uicsbd, PPCUIC_OUTPUT_CINT,
-                       qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_CINT));
+    if (!sysbus_realize(SYS_BUS_DEVICE(s->uic), errp)) {
+        return;
+    }
 
-    *uicdevp = uicdev;
+    sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_INT,
+                       qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_INT));
+    sysbus_connect_irq(SYS_BUS_DEVICE(s->uic), PPCUIC_OUTPUT_CINT,
+                       qdev_get_gpio_in(DEVICE(s->cpu), PPC40x_INPUT_CINT));
 
     /* SDRAM controller */
-        /* XXX 405EP has no ECC interrupt */
-    ppc4xx_sdram_init(env, qdev_get_gpio_in(uicdev, 17), 2, ram_memories,
-                      ram_bases, ram_sizes, do_init);
+    /* XXX 405EP has no ECC interrupt */
+    ppc4xx_sdram_init(env, qdev_get_gpio_in(s->uic, 17), 2, s->ram_memories,
+                      s->ram_bases, s->ram_sizes, s->do_dram_init);
+
     /* External bus controller */
     ppc405_ebc_init(env);
+
     /* DMA controller */
-    dma_irqs[0] = qdev_get_gpio_in(uicdev, 5);
-    dma_irqs[1] = qdev_get_gpio_in(uicdev, 6);
-    dma_irqs[2] = qdev_get_gpio_in(uicdev, 7);
-    dma_irqs[3] = qdev_get_gpio_in(uicdev, 8);
+    dma_irqs[0] = qdev_get_gpio_in(s->uic, 5);
+    dma_irqs[1] = qdev_get_gpio_in(s->uic, 6);
+    dma_irqs[2] = qdev_get_gpio_in(s->uic, 7);
+    dma_irqs[3] = qdev_get_gpio_in(s->uic, 8);
     ppc405_dma_init(env, dma_irqs);
-    /* IIC controller */
+
+    /* I2C controller */
     sysbus_create_simple(TYPE_PPC4xx_I2C, 0xef600500,
-                         qdev_get_gpio_in(uicdev, 2));
+                         qdev_get_gpio_in(s->uic, 2));
     /* GPIO */
     ppc405_gpio_init(0xef600700);
+
     /* Serial ports */
     if (serial_hd(0) != NULL) {
-        serial_mm_init(address_space_mem, 0xef600300, 0,
-                       qdev_get_gpio_in(uicdev, 0),
+        serial_mm_init(get_system_memory(), 0xef600300, 0,
+                       qdev_get_gpio_in(s->uic, 0),
                        PPC_SERIAL_MM_BAUDBASE, serial_hd(0),
                        DEVICE_BIG_ENDIAN);
     }
     if (serial_hd(1) != NULL) {
-        serial_mm_init(address_space_mem, 0xef600400, 0,
-                       qdev_get_gpio_in(uicdev, 1),
+        serial_mm_init(get_system_memory(), 0xef600400, 0,
+                       qdev_get_gpio_in(s->uic, 1),
                        PPC_SERIAL_MM_BAUDBASE, serial_hd(1),
                        DEVICE_BIG_ENDIAN);
     }
+
     /* OCM */
     ppc405_ocm_init(env);
+
     /* GPT */
-    gpt_irqs[0] = qdev_get_gpio_in(uicdev, 19);
-    gpt_irqs[1] = qdev_get_gpio_in(uicdev, 20);
-    gpt_irqs[2] = qdev_get_gpio_in(uicdev, 21);
-    gpt_irqs[3] = qdev_get_gpio_in(uicdev, 22);
-    gpt_irqs[4] = qdev_get_gpio_in(uicdev, 23);
+    gpt_irqs[0] = qdev_get_gpio_in(s->uic, 19);
+    gpt_irqs[1] = qdev_get_gpio_in(s->uic, 20);
+    gpt_irqs[2] = qdev_get_gpio_in(s->uic, 21);
+    gpt_irqs[3] = qdev_get_gpio_in(s->uic, 22);
+    gpt_irqs[4] = qdev_get_gpio_in(s->uic, 23);
     ppc4xx_gpt_init(0xef600000, gpt_irqs);
-    /* PCI */
-    /* Uses UIC IRQs 3, 16, 18 */
+
     /* MAL */
-    mal_irqs[0] = qdev_get_gpio_in(uicdev, 11);
-    mal_irqs[1] = qdev_get_gpio_in(uicdev, 12);
-    mal_irqs[2] = qdev_get_gpio_in(uicdev, 13);
-    mal_irqs[3] = qdev_get_gpio_in(uicdev, 14);
+    mal_irqs[0] = qdev_get_gpio_in(s->uic, 11);
+    mal_irqs[1] = qdev_get_gpio_in(s->uic, 12);
+    mal_irqs[2] = qdev_get_gpio_in(s->uic, 13);
+    mal_irqs[3] = qdev_get_gpio_in(s->uic, 14);
     ppc4xx_mal_init(env, 4, 2, mal_irqs);
+
     /* Ethernet */
     /* Uses UIC IRQs 9, 15, 17 */
-    /* CPU control */
-    ppc405ep_cpc_init(env, clk_setup, sysclk);
-
-    return cpu;
-}
-
-static void ppc405_soc_realize(DeviceState *dev, Error **errp)
-{
-    Ppc405SoCState *s = PPC405_SOC(dev);
-    Error *err = NULL;
-
-    /* XXX: fix this ? */
-    memory_region_init_alias(&s->ram_memories[0], OBJECT(s),
-                             "ef405ep.ram.alias", s->dram_mr, 0, s->ram_size);
-    s->ram_bases[0] = 0;
-    s->ram_sizes[0] = s->ram_size;
-    memory_region_init(&s->ram_memories[1], OBJECT(s), "ef405ep.ram1", 0);
-    s->ram_bases[1] = 0x00000000;
-    s->ram_sizes[1] = 0x00000000;
-
-    /* allocate SRAM */
-    memory_region_init_ram(&s->sram, OBJECT(s), "ef405ep.sram",
-                           PPC405EP_SRAM_SIZE,  &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    memory_region_add_subregion(get_system_memory(), PPC405EP_SRAM_BASE,
-                                &s->sram);
 }
 
 static Property ppc405_soc_properties[] = {
     DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
                      MemoryRegion *),
+    DEFINE_PROP_UINT32("sys-clk", Ppc405SoCState, sysclk, 0),
+    DEFINE_PROP_BOOL("dram-init", Ppc405SoCState, do_dram_init, 0),
     DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };