qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cédric Le Goater
Subject: Re: [PATCH v3 03/20] ppc4xx_sdram: Get rid of the init RAM hack
Date: Sun, 18 Sep 2022 09:34:40 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.1

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]