diff mbox series

[v3,05/22] ppc/ppc405: Introduce a PPC405 SoC

Message ID 20220808102734.133084-6-clg@kaod.org
State New
Headers show
Series ppc: QOM'ify 405 board | expand

Commit Message

Cédric Le Goater Aug. 8, 2022, 10:27 a.m. UTC
It is an initial model to start QOMification of the PPC405 board.
QOM'ified devices will be reintroduced one by one. Start with the
memory regions, which name prefix is changed to "ppc405".

Also, initialize only one RAM bank. The second bank is a dummy one
(zero size) which is here to match the hard coded number of banks in
ppc405ep_init().

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/ppc405.h        | 16 ++++++++++++++++
 hw/ppc/ppc405_boards.c | 23 ++++++++++++-----------
 hw/ppc/ppc405_uc.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 11 deletions(-)

Comments

BALATON Zoltan Aug. 8, 2022, 12:43 p.m. UTC | #1
On Mon, 8 Aug 2022, Cédric Le Goater wrote:
> It is an initial model to start QOMification of the PPC405 board.
> QOM'ified devices will be reintroduced one by one. Start with the
> memory regions, which name prefix is changed to "ppc405".
>
> Also, initialize only one RAM bank. The second bank is a dummy one
> (zero size) which is here to match the hard coded number of banks in
> ppc405ep_init().
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/ppc405.h        | 16 ++++++++++++++++
> hw/ppc/ppc405_boards.c | 23 ++++++++++++-----------
> hw/ppc/ppc405_uc.c     | 40 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 68 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index 83f156f585c8..66dc21cdfed8 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -25,6 +25,7 @@
> #ifndef PPC405_H
> #define PPC405_H
>
> +#include "qom/object.h"
> #include "hw/ppc/ppc4xx.h"
>
> #define PPC405EP_SDRAM_BASE 0x00000000
> @@ -62,6 +63,21 @@ struct ppc4xx_bd_info_t {
>     uint32_t bi_iic_fast[2];
> };
>
> +#define TYPE_PPC405_SOC "ppc405-soc"
> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405SoCState, PPC405_SOC);
> +
> +struct Ppc405SoCState {
> +    /* Private */
> +    DeviceState parent_obj;
> +
> +    /* Public */
> +    MemoryRegion ram_banks[2];
> +    hwaddr ram_bases[2], ram_sizes[2];
> +
> +    MemoryRegion *dram_mr;
> +    hwaddr ram_size;
> +};
> +
> /* PowerPC 405 core */
> ram_addr_t ppc405_set_bootinfo(CPUPPCState *env, ram_addr_t ram_size);
>
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index c6fa559b03d9..1dc5065fcc1d 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -57,6 +57,8 @@ struct Ppc405MachineState {
>     /* Private */
>     MachineState parent_obj;
>     /* Public */
> +
> +    Ppc405SoCState soc;
> };
>
> /*****************************************************************************/
> @@ -232,11 +234,10 @@ static void boot_from_kernel(MachineState *machine, PowerPCCPU *cpu)
>
> 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 *ram_memories = g_new(MemoryRegion, 2);
> -    hwaddr ram_bases[2], ram_sizes[2];
>     MemoryRegion *sysmem = get_system_memory();
>     DeviceState *uicdev;
>
> @@ -247,16 +248,16 @@ static void ppc405_init(MachineState *machine)
>         exit(EXIT_FAILURE);
>     }
>
> -    /* XXX: fix this */
> -    memory_region_init_alias(&ram_memories[0], NULL, "ef405ep.ram.alias",
> -                             machine->ram, 0, machine->ram_size);
> -    ram_bases[0] = 0;
> -    ram_sizes[0] = machine->ram_size;
> -    memory_region_init(&ram_memories[1], NULL, "ef405ep.ram1", 0);
> -    ram_bases[1] = 0x00000000;
> -    ram_sizes[1] = 0x00000000;
> +    object_initialize_child(OBJECT(machine), "soc", &ppc405->soc,
> +                            TYPE_PPC405_SOC);
> +    object_property_set_uint(OBJECT(&ppc405->soc), "ram-size",
> +                             machine->ram_size, &error_fatal);
> +    object_property_set_link(OBJECT(&ppc405->soc), "dram",
> +                             OBJECT(machine->ram), &error_abort);
> +    qdev_realize(DEVICE(&ppc405->soc), NULL, &error_abort);
>
> -    cpu = ppc405ep_init(sysmem, ram_memories, ram_bases, ram_sizes,
> +    cpu = ppc405ep_init(sysmem, ppc405->soc.ram_banks, ppc405->soc.ram_bases,
> +                        ppc405->soc.ram_sizes,
>                         33333333, &uicdev, kernel_filename == NULL ? 0 : 1);
>
>     /* allocate and load BIOS */
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index d6420c88d3a6..adadb3a0ae08 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -30,6 +30,7 @@
> #include "hw/ppc/ppc.h"
> #include "hw/i2c/ppc4xx_i2c.h"
> #include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> #include "ppc405.h"
> #include "hw/char/serial.h"
> #include "qemu/timer.h"
> @@ -1530,3 +1531,42 @@ PowerPCCPU *ppc405ep_init(MemoryRegion *address_space_mem,
>
>     return cpu;
> }
> +
> +static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> +{
> +    Ppc405SoCState *s = PPC405_SOC(dev);
> +
> +    /* Initialize only one bank */
> +    s->ram_bases[0] = 0;
> +    s->ram_sizes[0] = s->ram_size;
> +    memory_region_init_alias(&s->ram_banks[0], OBJECT(s),
> +                             "ppc405.sdram0", s->dram_mr,
> +                             s->ram_bases[0], s->ram_sizes[0]);
> +}
> +
> +static Property ppc405_soc_properties[] = {
> +    DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
> +                     MemoryRegion *),
> +    DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

I'm not sure why we need to duplicate these in the soc if they are always 
the same as machine->ram and machine->ram_size. Is it theoretically 
possible to have a soc where it uses ram that's not the whole ram? If not 
then you could just uose machine which is accessible either as 
current_machine or qdev_get_machine() (also the parent of out soc but we 
don't know how to get that). If setting ram this way is still desired do 
we separately need to set its size or we could use memory_region_size() 
instead?

> +
> +static void ppc405_soc_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc405_soc_realize;
> +    dc->user_creatable = false;

May need a comment explaining why user_creatable = false. (Also for all 
other similar lines in other patches, I won't repeat this there.)

Regards,
BALATON Zoltan

> +    device_class_set_props(dc, ppc405_soc_properties);
> +}
> +
> +static const TypeInfo ppc405_types[] = {
> +    {
> +        .name           = TYPE_PPC405_SOC,
> +        .parent         = TYPE_DEVICE,
> +        .instance_size  = sizeof(Ppc405SoCState),
> +        .class_init     = ppc405_soc_class_init,
> +    }
> +};
> +
> +DEFINE_TYPES(ppc405_types)
>
Cédric Le Goater Aug. 8, 2022, 1:51 p.m. UTC | #2
On 8/8/22 14:43, BALATON Zoltan wrote:
> On Mon, 8 Aug 2022, Cédric Le Goater wrote:
>> It is an initial model to start QOMification of the PPC405 board.
>> QOM'ified devices will be reintroduced one by one. Start with the
>> memory regions, which name prefix is changed to "ppc405".
>>
>> Also, initialize only one RAM bank. The second bank is a dummy one
>> (zero size) which is here to match the hard coded number of banks in
>> ppc405ep_init().
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/ppc/ppc405.h        | 16 ++++++++++++++++
>> hw/ppc/ppc405_boards.c | 23 ++++++++++++-----------
>> hw/ppc/ppc405_uc.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 68 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>> index 83f156f585c8..66dc21cdfed8 100644
>> --- a/hw/ppc/ppc405.h
>> +++ b/hw/ppc/ppc405.h
>> @@ -25,6 +25,7 @@
>> #ifndef PPC405_H
>> #define PPC405_H
>>
>> +#include "qom/object.h"
>> #include "hw/ppc/ppc4xx.h"
>>
>> #define PPC405EP_SDRAM_BASE 0x00000000
>> @@ -62,6 +63,21 @@ struct ppc4xx_bd_info_t {
>>     uint32_t bi_iic_fast[2];
>> };
>>
>> +#define TYPE_PPC405_SOC "ppc405-soc"
>> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405SoCState, PPC405_SOC);
>> +
>> +struct Ppc405SoCState {
>> +    /* Private */
>> +    DeviceState parent_obj;
>> +
>> +    /* Public */
>> +    MemoryRegion ram_banks[2];
>> +    hwaddr ram_bases[2], ram_sizes[2];
>> +
>> +    MemoryRegion *dram_mr;
>> +    hwaddr ram_size;
>> +};
>> +
>> /* PowerPC 405 core */
>> ram_addr_t ppc405_set_bootinfo(CPUPPCState *env, ram_addr_t ram_size);
>>
>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>> index c6fa559b03d9..1dc5065fcc1d 100644
>> --- a/hw/ppc/ppc405_boards.c
>> +++ b/hw/ppc/ppc405_boards.c
>> @@ -57,6 +57,8 @@ struct Ppc405MachineState {
>>     /* Private */
>>     MachineState parent_obj;
>>     /* Public */
>> +
>> +    Ppc405SoCState soc;
>> };
>>
>> /*****************************************************************************/
>> @@ -232,11 +234,10 @@ static void boot_from_kernel(MachineState *machine, PowerPCCPU *cpu)
>>
>> 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 *ram_memories = g_new(MemoryRegion, 2);
>> -    hwaddr ram_bases[2], ram_sizes[2];
>>     MemoryRegion *sysmem = get_system_memory();
>>     DeviceState *uicdev;
>>
>> @@ -247,16 +248,16 @@ static void ppc405_init(MachineState *machine)
>>         exit(EXIT_FAILURE);
>>     }
>>
>> -    /* XXX: fix this */
>> -    memory_region_init_alias(&ram_memories[0], NULL, "ef405ep.ram.alias",
>> -                             machine->ram, 0, machine->ram_size);
>> -    ram_bases[0] = 0;
>> -    ram_sizes[0] = machine->ram_size;
>> -    memory_region_init(&ram_memories[1], NULL, "ef405ep.ram1", 0);
>> -    ram_bases[1] = 0x00000000;
>> -    ram_sizes[1] = 0x00000000;
>> +    object_initialize_child(OBJECT(machine), "soc", &ppc405->soc,
>> +                            TYPE_PPC405_SOC);
>> +    object_property_set_uint(OBJECT(&ppc405->soc), "ram-size",
>> +                             machine->ram_size, &error_fatal);
>> +    object_property_set_link(OBJECT(&ppc405->soc), "dram",
>> +                             OBJECT(machine->ram), &error_abort);
>> +    qdev_realize(DEVICE(&ppc405->soc), NULL, &error_abort);
>>
>> -    cpu = ppc405ep_init(sysmem, ram_memories, ram_bases, ram_sizes,
>> +    cpu = ppc405ep_init(sysmem, ppc405->soc.ram_banks, ppc405->soc.ram_bases,
>> +                        ppc405->soc.ram_sizes,
>>                         33333333, &uicdev, kernel_filename == NULL ? 0 : 1);
>>
>>     /* allocate and load BIOS */
>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>> index d6420c88d3a6..adadb3a0ae08 100644
>> --- a/hw/ppc/ppc405_uc.c
>> +++ b/hw/ppc/ppc405_uc.c
>> @@ -30,6 +30,7 @@
>> #include "hw/ppc/ppc.h"
>> #include "hw/i2c/ppc4xx_i2c.h"
>> #include "hw/irq.h"
>> +#include "hw/qdev-properties.h"
>> #include "ppc405.h"
>> #include "hw/char/serial.h"
>> #include "qemu/timer.h"
>> @@ -1530,3 +1531,42 @@ PowerPCCPU *ppc405ep_init(MemoryRegion *address_space_mem,
>>
>>     return cpu;
>> }
>> +
>> +static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>> +{
>> +    Ppc405SoCState *s = PPC405_SOC(dev);
>> +
>> +    /* Initialize only one bank */
>> +    s->ram_bases[0] = 0;
>> +    s->ram_sizes[0] = s->ram_size;
>> +    memory_region_init_alias(&s->ram_banks[0], OBJECT(s),
>> +                             "ppc405.sdram0", s->dram_mr,
>> +                             s->ram_bases[0], s->ram_sizes[0]);
>> +}
>> +
>> +static Property ppc405_soc_properties[] = {
>> +    DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
>> +                     MemoryRegion *),
>> +    DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
> 
> I'm not sure why we need to duplicate these in the soc if they are always the same as machine->ram and machine->ram_size. 

There are machines with multi SoCs. PowerNV can have 16.

But that's not the reason here, we pass the dram memory region to the SoC
for controllers that might need it for memory transactions, typically DMAs.
In this case, it's the SDRAM controller which creates slices of RAM for
each available RAM bank.

> Is it theoretically possible to have a soc where it uses ram that's not the whole ram? 

Yes.

> If not then you could just uose machine which is accessible either as current_machine 
> or qdev_get_machine() 

These are QEMU globals. We should avoid using them in device models.

> (also the parent of out soc but we don't know how to get that). If setting ram this way
> is still desired do we separately need to set its size or we could use memory_region_size() 
> instead?

Let's keep SDRAM modeling for the next patchset.

  
>> +
>> +static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->realize = ppc405_soc_realize;
>> +    dc->user_creatable = false;
> 
> May need a comment explaining why user_creatable = false. (Also for all other similar lines in other patches, I won't repeat this there.)

TYPE_PPC405_SOC devices can not be created from the QEMU command line.
it doesn't make sense. That's what 'dc->user_creatable = false' means.

Thanks,

C.

> 
> Regards,
> BALATON Zoltan
> 
>> +    device_class_set_props(dc, ppc405_soc_properties);
>> +}
>> +
>> +static const TypeInfo ppc405_types[] = {
>> +    {
>> +        .name           = TYPE_PPC405_SOC,
>> +        .parent         = TYPE_DEVICE,
>> +        .instance_size  = sizeof(Ppc405SoCState),
>> +        .class_init     = ppc405_soc_class_init,
>> +    }
>> +};
>> +
>> +DEFINE_TYPES(ppc405_types)
>>
BALATON Zoltan Aug. 8, 2022, 2:02 p.m. UTC | #3
On Mon, 8 Aug 2022, Cédric Le Goater wrote:
> On 8/8/22 14:43, BALATON Zoltan wrote:
>> On Mon, 8 Aug 2022, Cédric Le Goater wrote:
>>> It is an initial model to start QOMification of the PPC405 board.
>>> QOM'ified devices will be reintroduced one by one. Start with the
>>> memory regions, which name prefix is changed to "ppc405".
>>> 
>>> Also, initialize only one RAM bank. The second bank is a dummy one
>>> (zero size) which is here to match the hard coded number of banks in
>>> ppc405ep_init().
>>> 
>>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>> hw/ppc/ppc405.h        | 16 ++++++++++++++++
>>> hw/ppc/ppc405_boards.c | 23 ++++++++++++-----------
>>> hw/ppc/ppc405_uc.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 68 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
>>> index 83f156f585c8..66dc21cdfed8 100644
>>> --- a/hw/ppc/ppc405.h
>>> +++ b/hw/ppc/ppc405.h
>>> @@ -25,6 +25,7 @@
>>> #ifndef PPC405_H
>>> #define PPC405_H
>>> 
>>> +#include "qom/object.h"
>>> #include "hw/ppc/ppc4xx.h"
>>> 
>>> #define PPC405EP_SDRAM_BASE 0x00000000
>>> @@ -62,6 +63,21 @@ struct ppc4xx_bd_info_t {
>>>     uint32_t bi_iic_fast[2];
>>> };
>>> 
>>> +#define TYPE_PPC405_SOC "ppc405-soc"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405SoCState, PPC405_SOC);
>>> +
>>> +struct Ppc405SoCState {
>>> +    /* Private */
>>> +    DeviceState parent_obj;
>>> +
>>> +    /* Public */
>>> +    MemoryRegion ram_banks[2];
>>> +    hwaddr ram_bases[2], ram_sizes[2];
>>> +
>>> +    MemoryRegion *dram_mr;
>>> +    hwaddr ram_size;
>>> +};
>>> +
>>> /* PowerPC 405 core */
>>> ram_addr_t ppc405_set_bootinfo(CPUPPCState *env, ram_addr_t ram_size);
>>> 
>>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>>> index c6fa559b03d9..1dc5065fcc1d 100644
>>> --- a/hw/ppc/ppc405_boards.c
>>> +++ b/hw/ppc/ppc405_boards.c
>>> @@ -57,6 +57,8 @@ struct Ppc405MachineState {
>>>     /* Private */
>>>     MachineState parent_obj;
>>>     /* Public */
>>> +
>>> +    Ppc405SoCState soc;
>>> };
>>> 
>>> /*****************************************************************************/
>>> @@ -232,11 +234,10 @@ static void boot_from_kernel(MachineState *machine, 
>>> PowerPCCPU *cpu)
>>> 
>>> 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 *ram_memories = g_new(MemoryRegion, 2);
>>> -    hwaddr ram_bases[2], ram_sizes[2];
>>>     MemoryRegion *sysmem = get_system_memory();
>>>     DeviceState *uicdev;
>>> 
>>> @@ -247,16 +248,16 @@ static void ppc405_init(MachineState *machine)
>>>         exit(EXIT_FAILURE);
>>>     }
>>> 
>>> -    /* XXX: fix this */
>>> -    memory_region_init_alias(&ram_memories[0], NULL, "ef405ep.ram.alias",
>>> -                             machine->ram, 0, machine->ram_size);
>>> -    ram_bases[0] = 0;
>>> -    ram_sizes[0] = machine->ram_size;
>>> -    memory_region_init(&ram_memories[1], NULL, "ef405ep.ram1", 0);
>>> -    ram_bases[1] = 0x00000000;
>>> -    ram_sizes[1] = 0x00000000;
>>> +    object_initialize_child(OBJECT(machine), "soc", &ppc405->soc,
>>> +                            TYPE_PPC405_SOC);
>>> +    object_property_set_uint(OBJECT(&ppc405->soc), "ram-size",
>>> +                             machine->ram_size, &error_fatal);
>>> +    object_property_set_link(OBJECT(&ppc405->soc), "dram",
>>> +                             OBJECT(machine->ram), &error_abort);
>>> +    qdev_realize(DEVICE(&ppc405->soc), NULL, &error_abort);
>>> 
>>> -    cpu = ppc405ep_init(sysmem, ram_memories, ram_bases, ram_sizes,
>>> +    cpu = ppc405ep_init(sysmem, ppc405->soc.ram_banks, 
>>> ppc405->soc.ram_bases,
>>> +                        ppc405->soc.ram_sizes,
>>>                         33333333, &uicdev, kernel_filename == NULL ? 0 : 
>>> 1);
>>> 
>>>     /* allocate and load BIOS */
>>> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
>>> index d6420c88d3a6..adadb3a0ae08 100644
>>> --- a/hw/ppc/ppc405_uc.c
>>> +++ b/hw/ppc/ppc405_uc.c
>>> @@ -30,6 +30,7 @@
>>> #include "hw/ppc/ppc.h"
>>> #include "hw/i2c/ppc4xx_i2c.h"
>>> #include "hw/irq.h"
>>> +#include "hw/qdev-properties.h"
>>> #include "ppc405.h"
>>> #include "hw/char/serial.h"
>>> #include "qemu/timer.h"
>>> @@ -1530,3 +1531,42 @@ PowerPCCPU *ppc405ep_init(MemoryRegion 
>>> *address_space_mem,
>>> 
>>>     return cpu;
>>> }
>>> +
>>> +static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    Ppc405SoCState *s = PPC405_SOC(dev);
>>> +
>>> +    /* Initialize only one bank */
>>> +    s->ram_bases[0] = 0;
>>> +    s->ram_sizes[0] = s->ram_size;
>>> +    memory_region_init_alias(&s->ram_banks[0], OBJECT(s),
>>> +                             "ppc405.sdram0", s->dram_mr,
>>> +                             s->ram_bases[0], s->ram_sizes[0]);
>>> +}
>>> +
>>> +static Property ppc405_soc_properties[] = {
>>> +    DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
>>> +                     MemoryRegion *),
>>> +    DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>> 
>> I'm not sure why we need to duplicate these in the soc if they are always 
>> the same as machine->ram and machine->ram_size. 
>
> There are machines with multi SoCs. PowerNV can have 16.
>
> But that's not the reason here, we pass the dram memory region to the SoC
> for controllers that might need it for memory transactions, typically DMAs.
> In this case, it's the SDRAM controller which creates slices of RAM for
> each available RAM bank.
>
>> Is it theoretically possible to have a soc where it uses ram that's not the 
>> whole ram? 
>
> Yes.
>
>> If not then you could just uose machine which is accessible either as 
>> current_machine or qdev_get_machine() 
>
> These are QEMU globals. We should avoid using them in device models.
>
>> (also the parent of out soc but we don't know how to get that). If setting 
>> ram this way
>> is still desired do we separately need to set its size or we could use 
>> memory_region_size() instead?
>
> Let's keep SDRAM modeling for the next patchset.

OK.

> 
>>> +
>>> +static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>> +
>>> +    dc->realize = ppc405_soc_realize;
>>> +    dc->user_creatable = false;
>> 
>> May need a comment explaining why user_creatable = false. (Also for all 
>> other similar lines in other patches, I won't repeat this there.)
>
> TYPE_PPC405_SOC devices can not be created from the QEMU command line.
> it doesn't make sense. That's what 'dc->user_creatable = false' means.

I know but near the definition of user_creatable there's a comment 
demanding that each occurance has to have a comment explaining why.

Regards,
BALATON Zoltan

> Thanks,
>
> C.
>
>> 
>> Regards,
>> BALATON Zoltan
>> 
>>> +    device_class_set_props(dc, ppc405_soc_properties);
>>> +}
>>> +
>>> +static const TypeInfo ppc405_types[] = {
>>> +    {
>>> +        .name           = TYPE_PPC405_SOC,
>>> +        .parent         = TYPE_DEVICE,
>>> +        .instance_size  = sizeof(Ppc405SoCState),
>>> +        .class_init     = ppc405_soc_class_init,
>>> +    }
>>> +};
>>> +
>>> +DEFINE_TYPES(ppc405_types)
>>> 
>
>
diff mbox series

Patch

diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
index 83f156f585c8..66dc21cdfed8 100644
--- a/hw/ppc/ppc405.h
+++ b/hw/ppc/ppc405.h
@@ -25,6 +25,7 @@ 
 #ifndef PPC405_H
 #define PPC405_H
 
+#include "qom/object.h"
 #include "hw/ppc/ppc4xx.h"
 
 #define PPC405EP_SDRAM_BASE 0x00000000
@@ -62,6 +63,21 @@  struct ppc4xx_bd_info_t {
     uint32_t bi_iic_fast[2];
 };
 
+#define TYPE_PPC405_SOC "ppc405-soc"
+OBJECT_DECLARE_SIMPLE_TYPE(Ppc405SoCState, PPC405_SOC);
+
+struct Ppc405SoCState {
+    /* Private */
+    DeviceState parent_obj;
+
+    /* Public */
+    MemoryRegion ram_banks[2];
+    hwaddr ram_bases[2], ram_sizes[2];
+
+    MemoryRegion *dram_mr;
+    hwaddr ram_size;
+};
+
 /* PowerPC 405 core */
 ram_addr_t ppc405_set_bootinfo(CPUPPCState *env, ram_addr_t ram_size);
 
diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index c6fa559b03d9..1dc5065fcc1d 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -57,6 +57,8 @@  struct Ppc405MachineState {
     /* Private */
     MachineState parent_obj;
     /* Public */
+
+    Ppc405SoCState soc;
 };
 
 /*****************************************************************************/
@@ -232,11 +234,10 @@  static void boot_from_kernel(MachineState *machine, PowerPCCPU *cpu)
 
 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 *ram_memories = g_new(MemoryRegion, 2);
-    hwaddr ram_bases[2], ram_sizes[2];
     MemoryRegion *sysmem = get_system_memory();
     DeviceState *uicdev;
 
@@ -247,16 +248,16 @@  static void ppc405_init(MachineState *machine)
         exit(EXIT_FAILURE);
     }
 
-    /* XXX: fix this */
-    memory_region_init_alias(&ram_memories[0], NULL, "ef405ep.ram.alias",
-                             machine->ram, 0, machine->ram_size);
-    ram_bases[0] = 0;
-    ram_sizes[0] = machine->ram_size;
-    memory_region_init(&ram_memories[1], NULL, "ef405ep.ram1", 0);
-    ram_bases[1] = 0x00000000;
-    ram_sizes[1] = 0x00000000;
+    object_initialize_child(OBJECT(machine), "soc", &ppc405->soc,
+                            TYPE_PPC405_SOC);
+    object_property_set_uint(OBJECT(&ppc405->soc), "ram-size",
+                             machine->ram_size, &error_fatal);
+    object_property_set_link(OBJECT(&ppc405->soc), "dram",
+                             OBJECT(machine->ram), &error_abort);
+    qdev_realize(DEVICE(&ppc405->soc), NULL, &error_abort);
 
-    cpu = ppc405ep_init(sysmem, ram_memories, ram_bases, ram_sizes,
+    cpu = ppc405ep_init(sysmem, ppc405->soc.ram_banks, ppc405->soc.ram_bases,
+                        ppc405->soc.ram_sizes,
                         33333333, &uicdev, kernel_filename == NULL ? 0 : 1);
 
     /* allocate and load BIOS */
diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
index d6420c88d3a6..adadb3a0ae08 100644
--- a/hw/ppc/ppc405_uc.c
+++ b/hw/ppc/ppc405_uc.c
@@ -30,6 +30,7 @@ 
 #include "hw/ppc/ppc.h"
 #include "hw/i2c/ppc4xx_i2c.h"
 #include "hw/irq.h"
+#include "hw/qdev-properties.h"
 #include "ppc405.h"
 #include "hw/char/serial.h"
 #include "qemu/timer.h"
@@ -1530,3 +1531,42 @@  PowerPCCPU *ppc405ep_init(MemoryRegion *address_space_mem,
 
     return cpu;
 }
+
+static void ppc405_soc_realize(DeviceState *dev, Error **errp)
+{
+    Ppc405SoCState *s = PPC405_SOC(dev);
+
+    /* Initialize only one bank */
+    s->ram_bases[0] = 0;
+    s->ram_sizes[0] = s->ram_size;
+    memory_region_init_alias(&s->ram_banks[0], OBJECT(s),
+                             "ppc405.sdram0", s->dram_mr,
+                             s->ram_bases[0], s->ram_sizes[0]);
+}
+
+static Property ppc405_soc_properties[] = {
+    DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
+                     MemoryRegion *),
+    DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ppc405_soc_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = ppc405_soc_realize;
+    dc->user_creatable = false;
+    device_class_set_props(dc, ppc405_soc_properties);
+}
+
+static const TypeInfo ppc405_types[] = {
+    {
+        .name           = TYPE_PPC405_SOC,
+        .parent         = TYPE_DEVICE,
+        .instance_size  = sizeof(Ppc405SoCState),
+        .class_init     = ppc405_soc_class_init,
+    }
+};
+
+DEFINE_TYPES(ppc405_types)