Re: [PATCH 10/20] ppc440_sdram: Implement enable bit in the DDR2 SDRAM controller

[BALATON Zoltan]

On Sun, 4 Sep 2022, Philippe Mathieu-Daudé via wrote:
> On 19/8/22 18:55, BALATON Zoltan wrote:
> > To allow removing the do_init hack we need to improve the DDR2 SDRAM
> > controller model to handle the enable/disable bit that it ignored so
> > far.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >   hw/ppc/ppc440_uc.c | 34 ++++++++++++++++++++++++++++++++--
> >   1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> > index c33f91e134..7c1513ff69 100644
> > --- a/hw/ppc/ppc440_uc.c
> > +++ b/hw/ppc/ppc440_uc.c
> > @@ -23,6 +23,7 @@
> >   #include "sysemu/reset.h"
> >   #include "ppc440.h"
> >   #include "qom/object.h"
> > +#include "trace.h"
> >     
> > /*****************************************************************************/
> >   /* L2 Cache as SRAM */
> > @@ -484,6 +485,7 @@ void ppc4xx_sdr_init(CPUPPCState *env)
> >   /* SDRAM controller */
> >   typedef struct ppc440_sdram_t {
> >       uint32_t addr;
> > +    uint32_t mcopt2;
> >       int nbanks;
> >       Ppc4xxSdramBank bank[4];
> >   } ppc440_sdram_t;
> > @@ -581,12 +583,15 @@ static void sdram_set_bcr(ppc440_sdram_t *sdram, int 
> > i,
> >   {
> >       if (sdram->bank[i].bcr & 1) {
> >           /* First unmap RAM if enabled */
> > +        trace_ppc4xx_sdram_unmap(sdram_base(sdram->bank[i].bcr),
> > +                                 sdram_size(sdram->bank[i].bcr));
>
> You extracted sdram_bank_unmap() for "reuse", so better
> add the trace event there.

These will end up there in patch 19 after consolidating ppc4xx DDR and 
ppc440 DDR2 models.

Thanks for your time reviewing it

Regards,
BALATON Zoltan

> >           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)) {
> > +        trace_ppc4xx_sdram_map(sdram_base(bcr), sdram_size(bcr));
>
> Ditto.
>
> >           sdram_bank_map(&sdram->bank[i]);
> >       }
> >   }