Re: [PATCH v3 03/20] ppc4xx_sdram: Get rid of the init RAM hack

[Cédric Le Goater]

On 9/13/22 21:52, BALATON Zoltan wrote:
> The do_init parameter of ppc4xx_sdram_init() is used to map memory
> regions that is normally done by the firmware by programming the SDRAM
> controller. This is needed when booting a kernel directly from -kernel
> without a firmware. Do this from board code accesing normal SDRAM

accessing

> controller registers the same way as firmware would do, so we can get
> rid of this hack.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v2: Fix ref405ep boot with -kernel and U-Boot
>
>   hw/ppc/ppc405.h         |  1 -
>   hw/ppc/ppc405_boards.c  | 12 ++++++++++--
>   hw/ppc/ppc405_uc.c      |  4 +---
>   hw/ppc/ppc440_bamboo.c  |  8 +++++++-
>   hw/ppc/ppc440_uc.c      |  2 --
>   hw/ppc/ppc4xx_devs.c    | 11 +----------
>   include/hw/ppc/ppc4xx.h |  8 ++++++--
>   7 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index 1e558c7831..756865621b 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -169,7 +169,6 @@ struct Ppc405SoCState {
>       /* Public */
>       MemoryRegion ram_banks[2];
>       hwaddr ram_bases[2], ram_sizes[2];
> -    bool do_dram_init;
>   
>       MemoryRegion *dram_mr;
>       hwaddr ram_size;
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 083f12b23e..bf02a71c6d 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -274,6 +274,7 @@ static void ppc405_init(MachineState *machine)
>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>       const char *kernel_filename = machine->kernel_filename;
>       MemoryRegion *sysmem = get_system_memory();
> +    CPUPPCState *env;
>   
>       if (machine->ram_size != mc->default_ram_size) {
>           char *sz = size_to_str(mc->default_ram_size);
> @@ -288,12 +289,19 @@ static void ppc405_init(MachineState *machine)
>                                machine->ram_size, &error_fatal);
>       object_property_set_link(OBJECT(&ppc405->soc), "dram",
>                                OBJECT(machine->ram), &error_abort);
> -    object_property_set_bool(OBJECT(&ppc405->soc), "dram-init",
> -                             kernel_filename != NULL, &error_abort);
>       object_property_set_uint(OBJECT(&ppc405->soc), "sys-clk", 33333333,
>                                &error_abort);
>       qdev_realize(DEVICE(&ppc405->soc), NULL, &error_fatal);
>   
> +    /* Enable SDRAM memory regions */
> +    /* FIXME This shouldn't be needed with firmware but we lack SPD data */

what do you mean ?

> +    env = &ppc405->soc.cpu.env;
> +    if (ppc_dcr_write(env->dcr_env, SDRAM0_CFGADDR, 0x20) ||
> +        ppc_dcr_write(env->dcr_env, SDRAM0_CFGDATA, 0x80000000)) {


I am not in favor of these ppc_drc_write calls and this is still a hack.

The "dram-init" property is a cleaner solution. It takes care of doing the
pre-mapping of RAM banks in the realize routine of the sdram model (when
available).


C.

> +        error_report("Could not enable memory regions");
> +        exit(1);
> +    }
> +
>       /* allocate and load BIOS */
>       if (machine->firmware) {
>           MemoryRegion *bios = g_new(MemoryRegion, 1);
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 2ca42fdef6..1e02347e57 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -1081,8 +1081,7 @@ static void ppc405_soc_realize(DeviceState *dev, Error 
> **errp)
>                                s->ram_bases[0], s->ram_sizes[0]);
>   
>       ppc4xx_sdram_init(env, qdev_get_gpio_in(DEVICE(&s->uic), 17), 1,
> -                      s->ram_banks, s->ram_bases, s->ram_sizes,
> -                      s->do_dram_init);
> +                      s->ram_banks, s->ram_bases, s->ram_sizes);
>   
>       /* External bus controller */
>       if (!ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(&s->ebc), &s->cpu, errp)) {
> @@ -1160,7 +1159,6 @@ static void ppc405_soc_realize(DeviceState *dev, Error 
> **errp)
>   static Property ppc405_soc_properties[] = {
>       DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
>                        MemoryRegion *),
> -    DEFINE_PROP_BOOL("dram-init", Ppc405SoCState, do_dram_init, 0),
>       DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
>       DEFINE_PROP_END_OF_LIST(),
>   };
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index 5ec82fa8c2..e3412c4fcd 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -211,7 +211,13 @@ static void bamboo_init(MachineState *machine)
>       ppc4xx_sdram_init(env,
>                         qdev_get_gpio_in(uicdev, 14),
>                         PPC440EP_SDRAM_NR_BANKS, ram_memories,
> -                      ram_bases, ram_sizes, 1);
> +                      ram_bases, ram_sizes);
> +    /* Enable SDRAM memory regions, this should be done by the firmware */
> +    if (ppc_dcr_write(env->dcr_env, SDRAM0_CFGADDR, 0x20) ||
> +        ppc_dcr_write(env->dcr_env, SDRAM0_CFGDATA, 0x80000000)) {
> +        error_report("couldn't enable memory regions");
> +        exit(1);
> +    }
>   
>       /* PCI */
>       dev = sysbus_create_varargs(TYPE_PPC4xx_PCI_HOST_BRIDGE,
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index db33334e29..6ab0ad7985 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -489,8 +489,6 @@ typedef struct ppc440_sdram_t {
>   } ppc440_sdram_t;
>   
>   enum {
> -    SDRAM0_CFGADDR = 0x10,
> -    SDRAM0_CFGDATA,
>       SDRAM_R0BAS = 0x40,
>       SDRAM_R1BAS,
>       SDRAM_R2BAS,
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 1226ec4aa9..936d6f77fe 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -56,11 +56,6 @@ struct ppc4xx_sdram_t {
>       qemu_irq irq;
>   };
>   
> -enum {
> -    SDRAM0_CFGADDR = 0x010,
> -    SDRAM0_CFGDATA = 0x011,
> -};
> -
>   /*
>    * XXX: TOFIX: some patches have made this code become inconsistent:
>    *      there are type inconsistencies, mixing hwaddr, target_ulong
> @@ -350,8 +345,7 @@ static void sdram_reset(void *opaque)
>   void ppc4xx_sdram_init(CPUPPCState *env, qemu_irq irq, int nbanks,
>                          MemoryRegion *ram_memories,
>                          hwaddr *ram_bases,
> -                       hwaddr *ram_sizes,
> -                       int do_init)
> +                       hwaddr *ram_sizes)
>   {
>       ppc4xx_sdram_t *sdram;
>       int i;
> @@ -369,9 +363,6 @@ void ppc4xx_sdram_init(CPUPPCState *env, qemu_irq irq, int 
> nbanks,
>                        sdram, &dcr_read_sdram, &dcr_write_sdram);
>       ppc_dcr_register(env, SDRAM0_CFGDATA,
>                        sdram, &dcr_read_sdram, &dcr_write_sdram);
> -    if (do_init) {
> -        sdram_map_bcr(sdram);
> -    }
>   }
>   
>   /*
> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> index 2af0d60577..a5e6c185af 100644
> --- a/include/hw/ppc/ppc4xx.h
> +++ b/include/hw/ppc/ppc4xx.h
> @@ -37,6 +37,11 @@ typedef struct {
>       uint32_t bcr;
>   } Ppc4xxSdramBank;
>   
> +enum {
> +    SDRAM0_CFGADDR = 0x010,
> +    SDRAM0_CFGDATA = 0x011,
> +};
> +
>   void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
>                           MemoryRegion ram_memories[],
>                           hwaddr ram_bases[], hwaddr ram_sizes[],
> @@ -45,8 +50,7 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
>   void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, int nbanks,
>                           MemoryRegion ram_memories[],
>                           hwaddr *ram_bases,
> -                        hwaddr *ram_sizes,
> -                        int do_init);
> +                        hwaddr *ram_sizes);
>   
>   #define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost"
>