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

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc440_uc.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

On 9/13/22 21:52, 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 5db59d1190..01184e717b 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);

This init belongs to the realize routine.

It is a oneliner. I think you can do it now without risks.

Thanks,

C.

> +    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));
> +}
> +
>   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]);
>       }
>   }
>

On Wed, 14 Sep 2022, Cédric Le Goater wrote:
> On 9/13/22 21:52, 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 5db59d1190..01184e717b 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);
>
> This init belongs to the realize routine.
>
> It is a oneliner. I think you can do it now without risks.

Not in this patch for sure as this is just moving this out from the write 
callback here. What you suggest is a separate clean up that could be 
considered as a followup after this series. I'm not sure which way is best 
as one could argue that we don't need this container region and could just 
create the ram region alias with the right size and offset but that may 
need to store more info or understand the register values better. Since I 
may not understand it completely I chose to not break it more and just 
leave it as it is for now. You could think about changing it afterwards 
separately.

Regards,
BALATON Zoltan

> Thanks,
>
> C.
>
>> +    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));
>> +}
>> +
>>   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]);
>>       }
>>   }
>> 
>
>
>