| Message ID | 554b4cde6c026bb7ba4bfbaa6d3e1e6019b40409.1663097286.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc4xx_sdram QOMify and clean ups | expand |
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" >
On Wed, 14 Sep 2022, Cédric Le Goater wrote: > 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 Fixed, also two ofhers in another patch you haven't noticed. >> 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 ? U-Boot detects the available RAM by reading the SPD info of the RAM modules but that probably also needs i2c emulation. See sam460ex. >> + 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. It's not. Normally this is done by firmware to enable memory controller but the board code has to do it if not using firmware (e.g. booting with -kernel) the same way it provides bootinfo or device tree mods the firmware would normally do or in this case maybe the emulation is incomplete so the part of firmware that configures the SDRAM controller does not run. > 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). I disagree, the hardware does not have such feature, it proviesd DCRs as the way to configure it. Adding a special property for it deviates from hardware and clutters qtree. Doing it like this patch is cleaner IMO. Regards, BALATON Zoltan > > 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" >> > > >
On 9/14/22 13:44, BALATON Zoltan wrote: > On Wed, 14 Sep 2022, Cédric Le Goater wrote: >> 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 > > Fixed, also two ofhers in another patch you haven't noticed. > >>> 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 ? > > U-Boot detects the available RAM by reading the SPD info of the RAM modules but that probably also needs i2c emulation. See sam460ex. > >>> + 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. > > It's not. Normally this is done by firmware to enable memory controller but the board code has to do it if not using firmware (e.g. booting with -kernel) the same way it provides bootinfo or device tree mods the firmware would normally do or in this case maybe the emulation is incomplete so the part of firmware that configures the SDRAM controller does not run. Exactly, and what the above proposal does is mimicking execution of CPU instructions before the CPU is even fully initiated. Reset has not been called at that stage. > >> 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). > > I disagree, the hardware does not have such feature, it proviesd DCRs as the way to configure it. Adding a special property for it deviates from hardware and clutters qtree. In this machine, running QEMU with -kernel deviates from HW. That's the whole purpose of this option. It assumes that the SDRAM device is pre-initialized (and much more should be done) by the QEMU machine and the simplest way to acheive this goal is to inform the SDRAM model to take care of the pre-initialization. Another way would be to change the default reset values of the SDRAM registers (in the realize method using some property) and perform some actions (mapping the banks) in the reset handler of the SDRAM device model. That would be a deferred initialization but a property is still needed to change the default behavior of the SDRAM model. Anyhow, this should be isolated under the SDRAM device model and not in the machine init by using the CPU state. That's clearly ugly. Thanks, C. > Doing it like this patch is cleaner IMO. > > Regards, > BALATON Zoltan > >> >> 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" >>> >> >> >>
On Wed, 14 Sep 2022, Cédric Le Goater wrote: > On 9/14/22 13:44, BALATON Zoltan wrote: >> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>> 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 >> >> Fixed, also two ofhers in another patch you haven't noticed. >> >>>> 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 ? >> >> U-Boot detects the available RAM by reading the SPD info of the RAM modules >> but that probably also needs i2c emulation. See sam460ex. >> >>>> + 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. >> >> It's not. Normally this is done by firmware to enable memory controller but >> the board code has to do it if not using firmware (e.g. booting with >> -kernel) the same way it provides bootinfo or device tree mods the firmware >> would normally do or in this case maybe the emulation is incomplete so the >> part of firmware that configures the SDRAM controller does not run. > > Exactly, and what the above proposal does is mimicking execution of CPU > instructions before the CPU is even fully initiated. Reset has not been > called at that stage. I don't get this. We're not calling any CPU instructions, ppc_dcr_write just calls the write callback the device has registered for the dcr so it just does the same as the hack did at the end just doing it the same way the firmware should do. >>> 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). >> >> I disagree, the hardware does not have such feature, it proviesd DCRs as >> the way to configure it. Adding a special property for it deviates from >> hardware and clutters qtree. > > > In this machine, running QEMU with -kernel deviates from HW. That's In all machines booting with -kernel likely deviates and all machines probably have additinal code in this case to do some things normally done by the firmware. Look at pegasos2_machine_reset() for example. All that is not needed when we boot with firmware as then the firmware will do all that and provide the device tree, etc. bur we need to do these when booting without firmware. In thes case QEMU also emulates the firmware and has to do thinigs like enabling the memory controller. > the whole purpose of this option. It assumes that the SDRAM device > is pre-initialized (and much more should be done) by the QEMU machine > and the simplest way to acheive this goal is to inform the SDRAM model > to take care of the pre-initialization. In my opinion the SDRAM controller model should model the hardware and if the board uses it differently then it should take care of that and not change the model. > Another way would be to change the default reset values of the SDRAM > registers (in the realize method using some property) and perform > some actions (mapping the banks) in the reset handler of the SDRAM > device model. That would be a deferred initialization but a property > is still needed to change the default behavior of the SDRAM model. > > Anyhow, this should be isolated under the SDRAM device model and > not in the machine init by using the CPU state. That's clearly ugly. Why? You already have the ppc405_set_bootinfo and all it's stuff in the ppc405 board which is also only needed without firmware. If you're opposed to the few lines enabling the memory controller being in ppc405_init I could put it in a function either in ppc405_boards.c or if you think this should be in ppc4xx_sdrem.c then we can export that function via include/hw/ppc/ppc4xx.h and call that from boards but I don't want to add hacks and a property for this in the device model becuase I'm not convinced it belongs there. If the hardware would have such an option then modeling that in is valid but if it's done by the firmare on the real hardware then either use the firmware or do it in board code which is then emulating the firmware too. Regards, BALATON Zoltan > > Thanks, > > C. > > > > > >> Doing it like this patch is cleaner IMO. >> >> Regards, >> BALATON Zoltan >> >>> >>> 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" >>>> >>> >>> >>> > > >
On Wed, 14 Sep 2022, BALATON Zoltan wrote: > On Wed, 14 Sep 2022, Cédric Le Goater wrote: >> On 9/14/22 13:44, BALATON Zoltan wrote: >>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>> 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 >>> >>> Fixed, also two ofhers in another patch you haven't noticed. >>> >>>>> 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 ? >>> >>> U-Boot detects the available RAM by reading the SPD info of the RAM >>> modules but that probably also needs i2c emulation. See sam460ex. >>> >>>>> + 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. >>> >>> It's not. Normally this is done by firmware to enable memory controller >>> but the board code has to do it if not using firmware (e.g. booting with >>> -kernel) the same way it provides bootinfo or device tree mods the >>> firmware would normally do or in this case maybe the emulation is >>> incomplete so the part of firmware that configures the SDRAM controller >>> does not run. >> >> Exactly, and what the above proposal does is mimicking execution of CPU >> instructions before the CPU is even fully initiated. Reset has not been >> called at that stage. > > I don't get this. We're not calling any CPU instructions, ppc_dcr_write just > calls the write callback the device has registered for the dcr so it just > does the same as the hack did at the end just doing it the same way the > firmware should do. > >>>> 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). >>> >>> I disagree, the hardware does not have such feature, it proviesd DCRs as >>> the way to configure it. Adding a special property for it deviates from >>> hardware and clutters qtree. >> >> >> In this machine, running QEMU with -kernel deviates from HW. That's > > In all machines booting with -kernel likely deviates and all machines > probably have additinal code in this case to do some things normally done by > the firmware. Look at pegasos2_machine_reset() for example. All that is not > needed when we boot with firmware as then the firmware will do all that and > provide the device tree, etc. bur we need to do these when booting without > firmware. In thes case QEMU also emulates the firmware and has to do thinigs > like enabling the memory controller. > >> the whole purpose of this option. It assumes that the SDRAM device >> is pre-initialized (and much more should be done) by the QEMU machine >> and the simplest way to acheive this goal is to inform the SDRAM model >> to take care of the pre-initialization. > > In my opinion the SDRAM controller model should model the hardware and if the > board uses it differently then it should take care of that and not change the > model. > >> Another way would be to change the default reset values of the SDRAM >> registers (in the realize method using some property) and perform >> some actions (mapping the banks) in the reset handler of the SDRAM >> device model. That would be a deferred initialization but a property >> is still needed to change the default behavior of the SDRAM model. >> >> Anyhow, this should be isolated under the SDRAM device model and >> not in the machine init by using the CPU state. That's clearly ugly. Additionally, if you don't like the FIXME comment, it's there because this would really belong at the beginning of boot_from_kernel() function before that calls ppc405_set_bootinfo which is called when booting without firmware but I left it where it was in init for now because you menfioned that firmware boot was also broken when I had it at the end of boot_from_kernel so I suspect the board is not providing the SPD data so the firmware cannot detect the RAM and this why it's not enabling the SDRAM itself (or maybe that part is even compiled out because of that) but then it's a limitation of the board emulation and not the SDRAM controller so it should be handled in the board code. Regards, BALATON Zoltan > Why? You already have the ppc405_set_bootinfo and all it's stuff in the > ppc405 board which is also only needed without firmware. If you're opposed to > the few lines enabling the memory controller being in ppc405_init I could put > it in a function either in ppc405_boards.c or if you think this should be in > ppc4xx_sdrem.c then we can export that function via include/hw/ppc/ppc4xx.h > and call that from boards but I don't want to add hacks and a property for > this in the device model becuase I'm not convinced it belongs there. If the > hardware would have such an option then modeling that in is valid but if it's > done by the firmare on the real hardware then either use the firmware or do > it in board code which is then emulating the firmware too. > > Regards, > BALATON Zoltan > >> >> Thanks, >> >> C. >> >> >> >> >> >>> Doing it like this patch is cleaner IMO. >>> >>> Regards, >>> BALATON Zoltan >>> >>>> >>>> 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" >>>>> >>>> >>>> >>>> >> >> >
On 9/14/22 20:25, BALATON Zoltan wrote: > On Wed, 14 Sep 2022, Cédric Le Goater wrote: >> On 9/14/22 13:44, BALATON Zoltan wrote: >>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>> 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 >>> >>> Fixed, also two ofhers in another patch you haven't noticed. >>> >>>>> 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 ? >>> >>> U-Boot detects the available RAM by reading the SPD info of the RAM modules but that probably also needs i2c emulation. See sam460ex. >>> >>>>> + 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. >>> >>> It's not. Normally this is done by firmware to enable memory controller but the board code has to do it if not using firmware (e.g. booting with -kernel) the same way it provides bootinfo or device tree mods the firmware would normally do or in this case maybe the emulation is incomplete so the part of firmware that configures the SDRAM controller does not run. >> >> Exactly, and what the above proposal does is mimicking execution of CPU >> instructions before the CPU is even fully initiated. Reset has not been >> called at that stage. > > I don't get this. We're not calling any CPU instructions, ppc_dcr_write just calls the write callback the device has registered for the dcr so it just does the same as the hack did at the end just doing it the same way the firmware should do. > >>>> 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). >>> >>> I disagree, the hardware does not have such feature, it proviesd DCRs as the way to configure it. Adding a special property for it deviates from hardware and clutters qtree. >> >> >> In this machine, running QEMU with -kernel deviates from HW. That's > > In all machines booting with -kernel likely deviates and all machines probably have additinal code in this case to do some things normally done by the firmware. Look at pegasos2_machine_reset() for example. All that is not needed when we boot with firmware as then the firmware will do all that and provide the device tree, etc. bur we need to do these when booting without firmware. In thes case QEMU also emulates the firmware and has to do thinigs like enabling the memory controller. > >> the whole purpose of this option. It assumes that the SDRAM device >> is pre-initialized (and much more should be done) by the QEMU machine >> and the simplest way to acheive this goal is to inform the SDRAM model >> to take care of the pre-initialization. > > In my opinion the SDRAM controller model should model the hardware and if the board uses it differently then it should take care of that and not change the model. > >> Another way would be to change the default reset values of the SDRAM >> registers (in the realize method using some property) and perform >> some actions (mapping the banks) in the reset handler of the SDRAM >> device model. That would be a deferred initialization but a property >> is still needed to change the default behavior of the SDRAM model. >> >> Anyhow, this should be isolated under the SDRAM device model and >> not in the machine init by using the CPU state. That's clearly ugly. > > Why? You already have the ppc405_set_bootinfo and all it's stuff in the ppc405 board which is also only needed without firmware. True. ppc405_set_bootinfo() is mimicking u-boot and updating RAM to pretend that FW already ran. It is done from under boot_from_kernel() only. FYI, The U-boot support was quite a mess : https://lists.ozlabs.org/pipermail/linuxppc-dev/2009-July/074487.html > If you're opposed to the few lines enabling the memory controller being in ppc405_init Well, that's what the init property is doing already. I do understand you need to move the code to cleanup the SDRAM model, but there are cleaner ways to do so. Alternative would be to remove the SDRAM pre-init for now, change the model and re-add it at the end when all is cleanly modified. The -kernel support would be temporarily broken but that's fine. On that topic, you should probably consider changing the patchset to propose first, a new SDRAM model (without using it in the boards) and then do the model "switch" at the end. That might be easier to review. > I could put it in a function either in ppc405_boards.c or if you think this should be in ppc4xx_sdrem.c > then we can export that function via include/hw/ppc/ppc4xx.h and call that from boards Yes. That would be better. Please call from under boot_from_kernel(), something like sdram_map_bcr(&ppc405->soc.sdram); > but I don't want to add hacks and a property for this in the device model becuase I'm not convinced it belongs there. That's how the default state of a model is changed and possibly taken into account at reset. A bit like HW strapping would work. Typically, in that case, you could use properties to change the BCR reg values and do the mapping accordingly at reset. > If the hardware would have such an option then modeling that in is valid QEMU has all kinds of extensions which makes it much more useful than HW in some places. The frontier is not that well draw. Thanks, C. > but if it's done by the firmare on the real hardware then either use the firmware or do it in board code which is then emulating the firmware too. > > Regards, > BALATON Zoltan > >> >> Thanks, >> >> C. >> >> >> >> >> >>> Doing it like this patch is cleaner IMO. >>> >>> Regards, >>> BALATON Zoltan >>> >>>> >>>> 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" >>>>> >>>> >>>> >>>> >> >> >>
On 9/14/22 20:32, BALATON Zoltan wrote: > On Wed, 14 Sep 2022, BALATON Zoltan wrote: >> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>> On 9/14/22 13:44, BALATON Zoltan wrote: >>>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>>> 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 >>>> >>>> Fixed, also two ofhers in another patch you haven't noticed. >>>> >>>>>> 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 ? >>>> >>>> U-Boot detects the available RAM by reading the SPD info of the RAM modules but that probably also needs i2c emulation. See sam460ex. >>>> >>>>>> + 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. >>>> >>>> It's not. Normally this is done by firmware to enable memory controller but the board code has to do it if not using firmware (e.g. booting with -kernel) the same way it provides bootinfo or device tree mods the firmware would normally do or in this case maybe the emulation is incomplete so the part of firmware that configures the SDRAM controller does not run. >>> >>> Exactly, and what the above proposal does is mimicking execution of CPU >>> instructions before the CPU is even fully initiated. Reset has not been >>> called at that stage. >> >> I don't get this. We're not calling any CPU instructions, ppc_dcr_write just calls the write callback the device has registered for the dcr so it just does the same as the hack did at the end just doing it the same way the firmware should do. >> >>>>> 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). >>>> >>>> I disagree, the hardware does not have such feature, it proviesd DCRs as the way to configure it. Adding a special property for it deviates from hardware and clutters qtree. >>> >>> >>> In this machine, running QEMU with -kernel deviates from HW. That's >> >> In all machines booting with -kernel likely deviates and all machines probably have additinal code in this case to do some things normally done by the firmware. Look at pegasos2_machine_reset() for example. All that is not needed when we boot with firmware as then the firmware will do all that and provide the device tree, etc. bur we need to do these when booting without firmware. In thes case QEMU also emulates the firmware and has to do thinigs like enabling the memory controller. >> >>> the whole purpose of this option. It assumes that the SDRAM device >>> is pre-initialized (and much more should be done) by the QEMU machine >>> and the simplest way to acheive this goal is to inform the SDRAM model >>> to take care of the pre-initialization. >> >> In my opinion the SDRAM controller model should model the hardware and if the board uses it differently then it should take care of that and not change the model. >> >>> Another way would be to change the default reset values of the SDRAM >>> registers (in the realize method using some property) and perform >>> some actions (mapping the banks) in the reset handler of the SDRAM >>> device model. That would be a deferred initialization but a property >>> is still needed to change the default behavior of the SDRAM model. >>> >>> Anyhow, this should be isolated under the SDRAM device model and >>> not in the machine init by using the CPU state. That's clearly ugly. > > Additionally, if you don't like the FIXME comment, I didn't understand it. That's different. > it's there because this would really belong at the beginning of boot_from_kernel() function before that calls ppc405_set_bootinfo which is called when booting without firmware but I left it where it was in init for now because you menfioned that firmware boot was also broken hmm ? but It's not anymore. v2 broke it I think. > when I had it at the end of boot_from_kernel so I suspect the board is not providing the SPD data so the firmware cannot detect the RAM and this why it's not enabling the SDRAM itself (or maybe that part is even compiled out because of that) but then it's a limitation of the board emulation and not the SDRAM controller so it should be handled in the board code. Here is the U-boot tree that can be used : https://gitlab.com/huth/u-boot/-/tree/taihu and the SDRAM init hacks : https://gitlab.com/huth/u-boot/-/commit/296e0479a972fa57390f0f3a912650168dabe851 May be there is a way to fix the model to remove the U-boot hack. Thomas added a uboot.bin in the tree which is used by : tests/avocado/ppc_405.py Thanks, C.
On Sun, 18 Sep 2022, Cédric Le Goater wrote: > On 9/14/22 20:32, BALATON Zoltan wrote: >> On Wed, 14 Sep 2022, BALATON Zoltan wrote: >>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>> On 9/14/22 13:44, BALATON Zoltan wrote: >>>>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>>>> 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 >>>>> >>>>> Fixed, also two ofhers in another patch you haven't noticed. >>>>> >>>>>>> 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 ? >>>>> >>>>> U-Boot detects the available RAM by reading the SPD info of the RAM >>>>> modules but that probably also needs i2c emulation. See sam460ex. >>>>> >>>>>>> + 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. >>>>> >>>>> It's not. Normally this is done by firmware to enable memory controller >>>>> but the board code has to do it if not using firmware (e.g. booting with >>>>> -kernel) the same way it provides bootinfo or device tree mods the >>>>> firmware would normally do or in this case maybe the emulation is >>>>> incomplete so the part of firmware that configures the SDRAM controller >>>>> does not run. >>>> >>>> Exactly, and what the above proposal does is mimicking execution of CPU >>>> instructions before the CPU is even fully initiated. Reset has not been >>>> called at that stage. >>> >>> I don't get this. We're not calling any CPU instructions, ppc_dcr_write >>> just calls the write callback the device has registered for the dcr so it >>> just does the same as the hack did at the end just doing it the same way >>> the firmware should do. >>> >>>>>> 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). >>>>> >>>>> I disagree, the hardware does not have such feature, it proviesd DCRs as >>>>> the way to configure it. Adding a special property for it deviates from >>>>> hardware and clutters qtree. >>>> >>>> >>>> In this machine, running QEMU with -kernel deviates from HW. That's >>> >>> In all machines booting with -kernel likely deviates and all machines >>> probably have additinal code in this case to do some things normally done >>> by the firmware. Look at pegasos2_machine_reset() for example. All that is >>> not needed when we boot with firmware as then the firmware will do all >>> that and provide the device tree, etc. bur we need to do these when >>> booting without firmware. In thes case QEMU also emulates the firmware and >>> has to do thinigs like enabling the memory controller. >>> >>>> the whole purpose of this option. It assumes that the SDRAM device >>>> is pre-initialized (and much more should be done) by the QEMU machine >>>> and the simplest way to acheive this goal is to inform the SDRAM model >>>> to take care of the pre-initialization. >>> >>> In my opinion the SDRAM controller model should model the hardware and if >>> the board uses it differently then it should take care of that and not >>> change the model. >>> >>>> Another way would be to change the default reset values of the SDRAM >>>> registers (in the realize method using some property) and perform >>>> some actions (mapping the banks) in the reset handler of the SDRAM >>>> device model. That would be a deferred initialization but a property >>>> is still needed to change the default behavior of the SDRAM model. >>>> >>>> Anyhow, this should be isolated under the SDRAM device model and >>>> not in the machine init by using the CPU state. That's clearly ugly. >> >> Additionally, if you don't like the FIXME comment, > > I didn't understand it. That's different. > >> it's there because this would really belong at the beginning of >> boot_from_kernel() function before that calls ppc405_set_bootinfo which is >> called when booting without firmware but I left it where it was in init for >> now because you menfioned that firmware boot was also broken > > hmm ? but It's not anymore. v2 broke it I think. First I've put enabling SDRAM controller in boot_from_kernel at the end before qemu_register_reset but this is wrong as the bootinfo is written in RAM so it should be enabled before. So this should be done at the beginning of boot_from_kernel but you said my first version not only broke -kernel but also booting with firmware so that told me firmware also does not enable the SDRAM conroller itself (as now seen below it's disabled) so I moved enabling back to where it was in init. Ideally it should be done by the firmware if using that or QEMU at the beginning of boot_from_kernel when emulating firmwere but that does not work yet so that's why we have the FIXME comment to remind for this. >> when I had it at the end of boot_from_kernel so I suspect the board is not >> providing the SPD data so the firmware cannot detect the RAM and this why >> it's not enabling the SDRAM itself (or maybe that part is even compiled out >> because of that) but then it's a limitation of the board emulation and not >> the SDRAM controller so it should be handled in the board code. > > Here is the U-boot tree that can be used : > > https://gitlab.com/huth/u-boot/-/tree/taihu > > and the SDRAM init hacks : > https://gitlab.com/huth/u-boot/-/commit/296e0479a972fa57390f0f3a912650168dabe851 > > May be there is a way to fix the model to remove the U-boot hack. I don't know how that works for 405. For 440 used by Sam460ex u-boot reads the SPD EEPROMs to detect memmory size and configures the SDRAM DDR2 controller according to that but that needs i2c and SPD data as in sam460ex.c. The code you pointed to seems to try a fixed table of valid RAM sizes in mb0cf[] that is either taken from CONFIG_SYS_SDRAM_TABLE value or use default of 128, 64, 32, 16, 4 MB. Maybe the model can be changed to allow this check to succeed so it does not have to be disabled but I don't know what it really tries to do here. The SoC manual may also have some docs on the register which you could check if you have it. This controller should be the same as in some 440 with DDR SDRAM. I think there are three different memory controllers in these SoCs: SDRAM, DDR and DDR2. The 460EX has DDR2, the 440 in bamboo has DDR and I think this is backwards compatible with SDRAM in 405 but the 440 DDR controller has some more regs for ECC. Therefore in QEMU we mostly only model DDR2 and DDR and use DDR instead of SDRAM in 405 as its common regs are likely the same but I don't have docs to prove it, I've only seen docs on 440 DDR so it's just what I've found while doing this series. Anyway you should keep this in mind when changing the DDR model and also check bamboo firmware which I don't have. Regards, BALATON Zoltan > > Thomas added a uboot.bin in the tree which is used by : > > tests/avocado/ppc_405.py > > Thanks, > > C. > >
On Sun, 18 Sep 2022, BALATON Zoltan wrote: > On Sun, 18 Sep 2022, Cédric Le Goater wrote: >> On 9/14/22 20:32, BALATON Zoltan wrote: >>> On Wed, 14 Sep 2022, BALATON Zoltan wrote: >>>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>>> On 9/14/22 13:44, BALATON Zoltan wrote: >>>>>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>>>>> 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 >>>>>> >>>>>> Fixed, also two ofhers in another patch you haven't noticed. >>>>>> >>>>>>>> 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 ? >>>>>> >>>>>> U-Boot detects the available RAM by reading the SPD info of the RAM >>>>>> modules but that probably also needs i2c emulation. See sam460ex. >>>>>> >>>>>>>> + 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. >>>>>> >>>>>> It's not. Normally this is done by firmware to enable memory controller >>>>>> but the board code has to do it if not using firmware (e.g. booting >>>>>> with -kernel) the same way it provides bootinfo or device tree mods the >>>>>> firmware would normally do or in this case maybe the emulation is >>>>>> incomplete so the part of firmware that configures the SDRAM controller >>>>>> does not run. >>>>> >>>>> Exactly, and what the above proposal does is mimicking execution of CPU >>>>> instructions before the CPU is even fully initiated. Reset has not been >>>>> called at that stage. >>>> >>>> I don't get this. We're not calling any CPU instructions, ppc_dcr_write >>>> just calls the write callback the device has registered for the dcr so it >>>> just does the same as the hack did at the end just doing it the same way >>>> the firmware should do. >>>> >>>>>>> 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). >>>>>> >>>>>> I disagree, the hardware does not have such feature, it proviesd DCRs >>>>>> as the way to configure it. Adding a special property for it deviates >>>>>> from hardware and clutters qtree. >>>>> >>>>> >>>>> In this machine, running QEMU with -kernel deviates from HW. That's >>>> >>>> In all machines booting with -kernel likely deviates and all machines >>>> probably have additinal code in this case to do some things normally done >>>> by the firmware. Look at pegasos2_machine_reset() for example. All that >>>> is not needed when we boot with firmware as then the firmware will do all >>>> that and provide the device tree, etc. bur we need to do these when >>>> booting without firmware. In thes case QEMU also emulates the firmware >>>> and has to do thinigs like enabling the memory controller. >>>> >>>>> the whole purpose of this option. It assumes that the SDRAM device >>>>> is pre-initialized (and much more should be done) by the QEMU machine >>>>> and the simplest way to acheive this goal is to inform the SDRAM model >>>>> to take care of the pre-initialization. >>>> >>>> In my opinion the SDRAM controller model should model the hardware and if >>>> the board uses it differently then it should take care of that and not >>>> change the model. >>>> >>>>> Another way would be to change the default reset values of the SDRAM >>>>> registers (in the realize method using some property) and perform >>>>> some actions (mapping the banks) in the reset handler of the SDRAM >>>>> device model. That would be a deferred initialization but a property >>>>> is still needed to change the default behavior of the SDRAM model. >>>>> >>>>> Anyhow, this should be isolated under the SDRAM device model and >>>>> not in the machine init by using the CPU state. That's clearly ugly. >>> >>> Additionally, if you don't like the FIXME comment, >> >> I didn't understand it. That's different. >> >>> it's there because this would really belong at the beginning of >>> boot_from_kernel() function before that calls ppc405_set_bootinfo which is >>> called when booting without firmware but I left it where it was in init >>> for now because you menfioned that firmware boot was also broken >> >> hmm ? but It's not anymore. v2 broke it I think. > > First I've put enabling SDRAM controller in boot_from_kernel at the end > before qemu_register_reset but this is wrong as the bootinfo is written in > RAM so it should be enabled before. So this should be done at the beginning > of boot_from_kernel but you said my first version not only broke -kernel but > also booting with firmware so that told me firmware also does not enable the > SDRAM conroller itself (as now seen below it's disabled) so I moved enabling On second look, enabling the memory controller in line 195 is not #ifdef-ed out so maybe it would work with enable only at the beginning of boot_from_kernel, I'll try to test with the firmware. For removing the hack the register values would need to be checked but that's something I don't want to do. You could look at that later as a follow up. Regards, BALATON Zoltan > back to where it was in init. Ideally it should be done by the firmware if > using that or QEMU at the beginning of boot_from_kernel when emulating > firmwere but that does not work yet so that's why we have the FIXME comment > to remind for this. > >>> when I had it at the end of boot_from_kernel so I suspect the board is not >>> providing the SPD data so the firmware cannot detect the RAM and this why >>> it's not enabling the SDRAM itself (or maybe that part is even compiled >>> out because of that) but then it's a limitation of the board emulation and >>> not the SDRAM controller so it should be handled in the board code. >> >> Here is the U-boot tree that can be used : >> >> https://gitlab.com/huth/u-boot/-/tree/taihu >> >> and the SDRAM init hacks : >> https://gitlab.com/huth/u-boot/-/commit/296e0479a972fa57390f0f3a912650168dabe851 >> >> May be there is a way to fix the model to remove the U-boot hack. > > I don't know how that works for 405. For 440 used by Sam460ex u-boot reads > the SPD EEPROMs to detect memmory size and configures the SDRAM DDR2 > controller according to that but that needs i2c and SPD data as in > sam460ex.c. The code you pointed to seems to try a fixed table of valid RAM > sizes in mb0cf[] that is either taken from CONFIG_SYS_SDRAM_TABLE value or > use default of 128, 64, 32, 16, 4 MB. Maybe the model can be changed to allow > this check to succeed so it does not have to be disabled but I don't know > what it really tries to do here. The SoC manual may also have some docs on > the register which you could check if you have it. This controller should be > the same as in some 440 with DDR SDRAM. I think there are three different > memory controllers in these SoCs: SDRAM, DDR and DDR2. The 460EX has DDR2, > the 440 in bamboo has DDR and I think this is backwards compatible with SDRAM > in 405 but the 440 DDR controller has some more regs for ECC. Therefore in > QEMU we mostly only model DDR2 and DDR and use DDR instead of SDRAM in 405 as > its common regs are likely the same but I don't have docs to prove it, I've > only seen docs on 440 DDR so it's just what I've found while doing this > series. Anyway you should keep this in mind when changing the DDR model and > also check bamboo firmware which I don't have. > > Regards, > BALATON Zoltan > >> >> Thomas added a uboot.bin in the tree which is used by : >> >> tests/avocado/ppc_405.py >> >> Thanks, >> >> C. >> >
On Sun, 18 Sep 2022, Cédric Le Goater wrote: > On 9/14/22 20:25, BALATON Zoltan wrote: >> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>> On 9/14/22 13:44, BALATON Zoltan wrote: >>>> On Wed, 14 Sep 2022, Cédric Le Goater wrote: >>>>> 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 >>>> >>>> Fixed, also two ofhers in another patch you haven't noticed. >>>> >>>>>> 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 ? >>>> >>>> U-Boot detects the available RAM by reading the SPD info of the RAM >>>> modules but that probably also needs i2c emulation. See sam460ex. >>>> >>>>>> + 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. >>>> >>>> It's not. Normally this is done by firmware to enable memory controller >>>> but the board code has to do it if not using firmware (e.g. booting with >>>> -kernel) the same way it provides bootinfo or device tree mods the >>>> firmware would normally do or in this case maybe the emulation is >>>> incomplete so the part of firmware that configures the SDRAM controller >>>> does not run. >>> >>> Exactly, and what the above proposal does is mimicking execution of CPU >>> instructions before the CPU is even fully initiated. Reset has not been >>> called at that stage. >> >> I don't get this. We're not calling any CPU instructions, ppc_dcr_write >> just calls the write callback the device has registered for the dcr so it >> just does the same as the hack did at the end just doing it the same way >> the firmware should do. >> >>>>> 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). >>>> >>>> I disagree, the hardware does not have such feature, it proviesd DCRs as >>>> the way to configure it. Adding a special property for it deviates from >>>> hardware and clutters qtree. >>> >>> >>> In this machine, running QEMU with -kernel deviates from HW. That's >> >> In all machines booting with -kernel likely deviates and all machines >> probably have additinal code in this case to do some things normally done >> by the firmware. Look at pegasos2_machine_reset() for example. All that is >> not needed when we boot with firmware as then the firmware will do all that >> and provide the device tree, etc. bur we need to do these when booting >> without firmware. In thes case QEMU also emulates the firmware and has to >> do thinigs like enabling the memory controller. >> >>> the whole purpose of this option. It assumes that the SDRAM device >>> is pre-initialized (and much more should be done) by the QEMU machine >>> and the simplest way to acheive this goal is to inform the SDRAM model >>> to take care of the pre-initialization. >> >> In my opinion the SDRAM controller model should model the hardware and if >> the board uses it differently then it should take care of that and not >> change the model. >> >>> Another way would be to change the default reset values of the SDRAM >>> registers (in the realize method using some property) and perform >>> some actions (mapping the banks) in the reset handler of the SDRAM >>> device model. That would be a deferred initialization but a property >>> is still needed to change the default behavior of the SDRAM model. >>> >>> Anyhow, this should be isolated under the SDRAM device model and >>> not in the machine init by using the CPU state. That's clearly ugly. >> >> Why? You already have the ppc405_set_bootinfo and all it's stuff in the >> ppc405 board which is also only needed without firmware. > > True. ppc405_set_bootinfo() is mimicking u-boot and updating RAM to pretend > that FW already ran. It is done from under boot_from_kernel() only. > > FYI, The U-boot support was quite a mess : > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2009-July/074487.html > >> If you're opposed to the few lines enabling the memory controller being in >> ppc405_init > > Well, that's what the init property is doing already. I do understand you > need to move the code to cleanup the SDRAM model, but there are cleaner > ways to do so. > > Alternative would be to remove the SDRAM pre-init for now, change the > model and re-add it at the end when all is cleanly modified. The -kernel > support would be temporarily broken but that's fine. Doing the init in any other way than through dcr write is bad becuause then the register values and mapped regions can get out of sync so I don't like to add any other hack to go around this. The code to enable and disable the contrller is there at one place and everything should go through that. > On that topic, you should probably consider changing the patchset to > propose first, a new SDRAM model (without using it in the boards) and > then do the model "switch" at the end. That might be easier to review. Besides that ir would require me to redo everthing from scratch it would also be hard to bisect as you ended up with one patch changing everything and no way to test the individual steps so I don't think this is a good idea. >> I could put it in a function either in ppc405_boards.c or if you think this >> should be in ppc4xx_sdrem.c then we can export that function via >> include/hw/ppc/ppc4xx.h and call that from boards > > Yes. That would be better. I went with this then in v5... > Please call from under boot_from_kernel(), something like > > sdram_map_bcr(&ppc405->soc.sdram); ...but not quite like this becuase of the above and also because this function will be gone by the end of the series. >> but I don't want to add hacks and a property for this in the device model >> becuase I'm not convinced it belongs there. > > That's how the default state of a model is changed and possibly taken > into account at reset. A bit like HW strapping would work. > > Typically, in that case, you could use properties to change the BCR > reg values and do the mapping accordingly at reset. I'm not a fan of qdev properties that turn a sysbus_create_simple() into 5 lines of unreadable code and also mess up -device help and info qtree output. (At least these devices are not used creatable so -device help is not a problem here.) >> If the hardware would have such an option then modeling that in is valid > > QEMU has all kinds of extensions which makes it much more useful than HW > in some places. The frontier is not that well draw. Sure but in this case I think it would not make it more useful and we could just model the device better without such an addition. Regards, BALATON Zoltan > Thanks, > > C. > > >> but if it's done by the firmare on the real hardware then either use the >> firmware or do it in board code which is then emulating the firmware too. >> >> Regards, >> BALATON Zoltan >> >>> >>> Thanks, >>> >>> C. >>> >>> >>> >>> >>> >>>> Doing it like this patch is cleaner IMO. >>>> >>>> Regards, >>>> BALATON Zoltan >>>> >>>>> >>>>> 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" >>>>>> >>>>> >>>>> >>>>> >>> >>> >>> > > >
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 */ + env = &ppc405->soc.cpu.env; + if (ppc_dcr_write(env->dcr_env, SDRAM0_CFGADDR, 0x20) || + ppc_dcr_write(env->dcr_env, SDRAM0_CFGDATA, 0x80000000)) { + 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"
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 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(-)