Re: [PATCH 09/20] ppc440_sdram: Split off map/unmap of sdram banks for later reuse

[Cédric Le Goater]

On 8/19/22 18:55, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/ppc440_uc.c | 31 +++++++++++++++++++------------
>   1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index 3507c35b63..c33f91e134 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -561,26 +561,33 @@ static uint64_t sdram_size(uint32_t bcr)
>       return size;
>   }
>   
> +static void sdram_bank_map(Ppc4xxSdramBank *bank)
> +{
> +    memory_region_init(&bank->container, NULL, "sdram-container", bank->size);

I don't think we need to init the ->container memory region each time.
This could be done once and for all in the realize handler.

> +    memory_region_add_subregion(&bank->container, 0, &bank->ram);
> +    memory_region_add_subregion(get_system_memory(), bank->base,
> +                                &bank->container);
> +}
> +
> +static void sdram_bank_unmap(Ppc4xxSdramBank *bank)
> +{
> +    memory_region_del_subregion(get_system_memory(), &bank->container);
> +    memory_region_del_subregion(&bank->container, &bank->ram);
> +    object_unparent(OBJECT(&bank->container));

object_unparent could be dropped if the memory_region_init was called in
realize.

Also, memory_region_set_enabled() might be a better alternative.

Thanks,

C.


> +}
> +
>   static void sdram_set_bcr(ppc440_sdram_t *sdram, int i,
>                             uint32_t bcr, int enabled)
>   {
>       if (sdram->bank[i].bcr & 1) {
>           /* First unmap RAM if enabled */
> -        memory_region_del_subregion(get_system_memory(),
> -                                    &sdram->bank[i].container);
> -        memory_region_del_subregion(&sdram->bank[i].container,
> -                                    &sdram->bank[i].ram);
> -        object_unparent(OBJECT(&sdram->bank[i].container));
> +        sdram_bank_unmap(&sdram->bank[i]);
>       }
>       sdram->bank[i].bcr = bcr & 0xffe0ffc1;
> +    sdram->bank[i].base = sdram_base(bcr);
> +    sdram->bank[i].size = sdram_size(bcr);
>       if (enabled && (bcr & 1)) {
> -        memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
> -                           sdram_size(bcr));
> -        memory_region_add_subregion(&sdram->bank[i].container, 0,
> -                                    &sdram->bank[i].ram);
> -        memory_region_add_subregion(get_system_memory(),
> -                                    sdram_base(bcr),
> -                                    &sdram->bank[i].container);
> +        sdram_bank_map(&sdram->bank[i]);
>       }
>   }
>