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

[BALATON Zoltan]

On Wed, 7 Sep 2022, Cédric Le Goater wrote:
> 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.

I think these could be considered as a follow up later, I don't want to 
change it now to avoid breaking it more as I've already managed to break 
ref405ep as you found (this will be fixed in v2) so I'd not try to change 
this in this series.

Regards,
BALATON Zoltan

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