Re: [PATCH v2 67/86] ppc:ppc440_bamboo/sam460ex: use memdev for RAM

[BALATON Zoltan]

On Wed, 15 Jan 2020, Igor Mammedov wrote:
> memory_region_allocate_system_memory() API is going away, so
> replace it with memdev allocated MemoryRegion. The later is
> initialized by generic code, so board only needs to opt in
> to memdev scheme by providing
>  MachineClass::default_ram_id
> and using MachineState::ram instead of manually initializing
> RAM memory region.
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> CC: address@hidden
> CC: address@hidden
> CC: address@hidden
> ---
> include/hw/ppc/ppc4xx.h | 2 +-
> hw/ppc/ppc440_bamboo.c  | 3 ++-
> hw/ppc/ppc4xx_devs.c    | 9 +++------
> hw/ppc/sam460ex.c       | 3 ++-
> 4 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> index 103c875..f0bef46 100644
> --- a/include/hw/ppc/ppc4xx.h
> +++ b/include/hw/ppc/ppc4xx.h
> @@ -42,7 +42,7 @@ enum {
> qemu_irq *ppcuic_init (CPUPPCState *env, qemu_irq *irqs,
>                        uint32_t dcr_base, int has_ssr, int has_vr);
>
> -void ppc4xx_sdram_prep(ram_addr_t ram_size, int nr_banks,
> +void ppc4xx_sdram_prep(MemoryRegion *ram, int nr_banks,
>                        MemoryRegion ram_memories[],
>                        hwaddr ram_bases[], hwaddr ram_sizes[],
>                        const ram_addr_t sdram_bank_sizes[]);
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index c162598..cb4a1ad 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -202,7 +202,7 @@ static void bamboo_init(MachineState *machine)
>     /* SDRAM controller */
>     memset(ram_bases, 0, sizeof(ram_bases));
>     memset(ram_sizes, 0, sizeof(ram_sizes));
> -    ppc4xx_sdram_prep(ram_size, PPC440EP_SDRAM_NR_BANKS, ram_memories,
> +    ppc4xx_sdram_prep(machine->ram, PPC440EP_SDRAM_NR_BANKS, ram_memories,
>                       ram_bases, ram_sizes, ppc440ep_sdram_bank_sizes);
>     /* XXX 440EP's ECC interrupts are on UIC1, but we've only created UIC0. */
>     ppc4xx_sdram_init(env, pic[14], PPC440EP_SDRAM_NR_BANKS, ram_memories,
> @@ -289,6 +289,7 @@ static void bamboo_machine_init(MachineClass *mc)
>     mc->desc = "bamboo";
>     mc->init = bamboo_init;
>     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("440epb");
> +    mc->default_ram_id = "ppc4xx.sdram";
> }
>
> DEFINE_MACHINE("bamboo", bamboo_machine_init)
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 92d33a4..9c3762d 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -673,13 +673,12 @@ void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, 
> int nbanks,
>  * The 4xx SDRAM controller supports a small number of banks, and each bank
>  * must be one of a small set of sizes. The number of banks and the supported
>  * sizes varies by SoC. */
> -void ppc4xx_sdram_prep(ram_addr_t ram_size, int nr_banks,
> +void ppc4xx_sdram_prep(MemoryRegion *ram, int nr_banks,

Maybe you need to also update the comment above which talks about 
'ram_size' that you don't have any more.

>                        MemoryRegion ram_memories[],
>                        hwaddr ram_bases[], hwaddr ram_sizes[],
>                        const ram_addr_t sdram_bank_sizes[])
> {
> -    MemoryRegion *ram = g_malloc0(sizeof(*ram));
> -    ram_addr_t size_left = ram_size;
> +    ram_addr_t size_left = memory_region_size(ram);
>     ram_addr_t base = 0;
>     ram_addr_t bank_size;
>     int last_bank = 0;
> @@ -704,7 +703,7 @@ void ppc4xx_sdram_prep(ram_addr_t ram_size, int nr_banks,
>     }
>
>     if (size_left) {
> -        ram_addr_t used_size = ram_size - size_left;
> +        ram_addr_t used_size = memory_region_size(ram) - size_left;
>         GString *s = g_string_new(NULL);
>
>         for (i = 0; sdram_bank_sizes[i]; i++) {
> @@ -721,8 +720,6 @@ void ppc4xx_sdram_prep(ram_addr_t ram_size, int nr_banks,
>         exit(EXIT_FAILURE);
>     }
>
> -    memory_region_allocate_system_memory(ram, NULL, "ppc4xx.sdram", ram_size);
> -
>     for (i = 0; i <= last_bank; i++) {

Can this for cycle now completely merged with the one above and init the 
alias regions also there?

Regards,
BALATON Zoltan

>         char name[32];
>         snprintf(name, sizeof(name), "ppc4xx.sdram%d", i);
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index ec7ac1f..8d27a3f 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -324,7 +324,7 @@ static void sam460ex_init(MachineState *machine)
>     /* SDRAM controller */
>     /* put all RAM on first bank because board has one slot
>      * and firmware only checks that */
> -    ppc4xx_sdram_prep(machine->ram_size, 1, ram_memories, ram_bases, ram_sizes,
> +    ppc4xx_sdram_prep(machine->ram, 1, ram_memories, ram_bases, ram_sizes,
>                       ppc460ex_sdram_bank_sizes);
>
>     /* FIXME: does 460EX have ECC interrupts? */
> @@ -483,6 +483,7 @@ static void sam460ex_machine_init(MachineClass *mc)
>     mc->init = sam460ex_init;
>     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("460exb");
>     mc->default_ram_size = 512 * MiB;
> +    mc->default_ram_id = "ppc4xx.sdram";
> }
>
> DEFINE_MACHINE("sam460ex", sam460ex_machine_init)