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

[Cédric Le Goater]

On 8/19/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));
>           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));
>           sdram_bank_map(&sdram->bank[i]);
>       }
>   }
> @@ -596,7 +601,7 @@ static void sdram_map_bcr(ppc440_sdram_t *sdram)
>       int i;
>   
>       for (i = 0; i < sdram->nbanks; i++) {
> -        if (sdram->bank[i].size != 0) {
> +        if (sdram->bank[i].size) {
>               sdram_set_bcr(sdram, i, sdram_bcr(sdram->bank[i].base,
>                                                 sdram->bank[i].size), 1);
>           } else {
> @@ -605,6 +610,17 @@ static void sdram_map_bcr(ppc440_sdram_t *sdram)
>       }
>   }
>   
> +static void sdram_unmap_bcr(ppc440_sdram_t *sdram)
> +{
> +    int i;
> +
> +    for (i = 0; i < sdram->nbanks; i++) {
> +        if (sdram->bank[i].size) {
> +            sdram_set_bcr(sdram, i, sdram->bank[i].bcr & ~1, 0);
> +        }
> +    }
> +}
> +
>   static uint32_t dcr_read_sdram(void *opaque, int dcrn)
>   {
>       ppc440_sdram_t *sdram = opaque;
> @@ -636,7 +652,7 @@ static uint32_t dcr_read_sdram(void *opaque, int dcrn)
>               ret = 0x80000000;
>               break;
>           case 0x21: /* SDRAM_MCOPT2 */
> -            ret = 0x08000000;
> +            ret = sdram->mcopt2;
>               break;
>           case 0x40: /* SDRAM_MB0CF */
>               ret = 0x00008001;
> @@ -680,6 +696,19 @@ static void dcr_write_sdram(void *opaque, int dcrn, 
> uint32_t val)
>           switch (sdram->addr) {
>           case 0x00: /* B0CR */
>               break;
> +        case 0x21: /* SDRAM_MCOPT2 */
> +            if (!(sdram->mcopt2 & 0x08000000) && (val & 0x08000000)) {
> +                trace_ppc4xx_sdram_enable("enable");
> +                /* validate all RAM mappings */
> +                sdram_map_bcr(sdram);
> +                sdram->mcopt2 |= 0x08000000;
> +            } else if ((sdram->mcopt2 & 0x08000000) && !(val & 0x08000000)) {
> +                trace_ppc4xx_sdram_enable("disable");
> +                /* invalidate all RAM mappings */
> +                sdram_unmap_bcr(sdram);
> +                sdram->mcopt2 &= ~0x08000000;
> +            }
> +            break;
>           default:
>               break;
>           }
> @@ -694,6 +723,7 @@ static void sdram_reset(void *opaque)
>       ppc440_sdram_t *sdram = opaque;
>   
>       sdram->addr = 0;
> +    sdram->mcopt2 = 0x08000000;

Introducing a define for the 0x08000000 value would make the code easier
to read.

Thanks,

C.

>   }
>   
>   void ppc440_sdram_init(CPUPPCState *env, int nbanks,