[PATCH 0/4] E500 cleanups and enhancements

[PATCH 0/4] E500 cleanups and enhancements

[Bernhard Beschow]

This series includes some cleanups I came across when working on the ppce500
machine. Furthermore, it enables support for the 'dumpdtb' QMP/HMP command
which was missing so far.

Bernhard Beschow (4):
  hw/ppc: Set machine->fdt in e500 machines
  hw/ppc/e500{,plat}: Drop redundant checks for presence of platform bus
  hw/ppc/e500.c: Avoid hardcoding parent device in
    create_devtree_etsec()
  hw/ppc/e500.c: Attach eSDHC unimplemented region to ccsr_addr_space

 hw/ppc/e500.c     | 24 ++++++++++++++++--------
 hw/ppc/e500plat.c |  9 +++------
 2 files changed, 19 insertions(+), 14 deletions(-)

-- 
2.39.1

[PATCH 1/4] hw/ppc: Set machine->fdt in e500 machines

[Bernhard Beschow]

This enables support for the 'dumpdtb' QMP/HMP command for all
e500 machines.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 9fa1f8e6cf..7239993acc 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -659,9 +659,14 @@ done:
     if (!dry_run) {
         qemu_fdt_dumpdtb(fdt, fdt_size);
         cpu_physical_memory_write(addr, fdt, fdt_size);
+
+        /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
+        g_free(machine->fdt);
+        machine->fdt = fdt;
+    } else {
+        g_free(fdt);
     }
     ret = fdt_size;
-    g_free(fdt);
 
 out:
     g_free(pci_map);
-- 
2.39.1

[PATCH 2/4] hw/ppc/e500{, plat}: Drop redundant checks for presence of platform bus

[Bernhard Beschow]

This is a follow-up on commit 47a0b1dff7e9 'hw/ppc/mpc8544ds: Add
platform bus': Both mpc85xx boards now have a platform bus
unconditionally.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c     | 5 ++---
 hw/ppc/e500plat.c | 9 +++------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 7239993acc..48288c0b41 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -643,9 +643,8 @@ static int ppce500_load_device_tree(PPCE500MachineState *pms,
     }
     g_free(soc);
 
-    if (pms->pbus_dev) {
-        platform_bus_create_devtree(pms, fdt, mpic);
-    }
+    platform_bus_create_devtree(pms, fdt, mpic);
+
     g_free(mpic);
 
     pmc->fixup_devtree(fdt);
diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c
index 44bf874b0f..3032bd3f6d 100644
--- a/hw/ppc/e500plat.c
+++ b/hw/ppc/e500plat.c
@@ -46,13 +46,10 @@ static void e500plat_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                             DeviceState *dev, Error **errp)
 {
     PPCE500MachineState *pms = PPCE500_MACHINE(hotplug_dev);
+    MachineClass *mc = MACHINE_GET_CLASS(pms);
 
-    if (pms->pbus_dev) {
-        MachineClass *mc = MACHINE_GET_CLASS(pms);
-
-        if (device_is_dynamic_sysbus(mc, dev)) {
-            platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
-        }
+    if (device_is_dynamic_sysbus(mc, dev)) {
+        platform_bus_link_device(pms->pbus_dev, SYS_BUS_DEVICE(dev));
     }
 }
 
-- 
2.39.1

[PATCH 3/4] hw/ppc/e500.c: Avoid hardcoding parent device in create_devtree_etsec()

[Bernhard Beschow]

The "platform" node is available through data->node, so use that instead
of making assumptions about the parent device.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 48288c0b41..e3b29d1d97 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -241,7 +241,7 @@ static int create_devtree_etsec(SysBusDevice *sbdev, PlatformDevtreeData *data)
     int irq0 = platform_bus_get_irqn(pbus, sbdev, 0);
     int irq1 = platform_bus_get_irqn(pbus, sbdev, 1);
     int irq2 = platform_bus_get_irqn(pbus, sbdev, 2);
-    gchar *node = g_strdup_printf("/platform/ethernet@%"PRIx64, mmio0);
+    gchar *node = g_strdup_printf("%s/ethernet@%"PRIx64, data->node, mmio0);
     gchar *group = g_strdup_printf("%s/queue-group", node);
     void *fdt = data->fdt;
 
-- 
2.39.1

[PATCH 4/4] hw/ppc/e500.c: Attach eSDHC unimplemented region to ccsr_addr_space

[Bernhard Beschow]

Makes the unimplemented region move together with the CCSR address space
if moved by a bootloader. Moving the CCSR address space isn't
implemented yet but this patch is a preparation for it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index e3b29d1d97..117c9c08ed 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1022,9 +1022,13 @@ void ppce500_init(MachineState *machine)
 
     /* eSDHC */
     if (pmc->has_esdhc) {
-        create_unimplemented_device("esdhc",
-                                    pmc->ccsrbar_base + MPC85XX_ESDHC_REGS_OFFSET,
-                                    MPC85XX_ESDHC_REGS_SIZE);
+        dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
+        qdev_prop_set_string(dev, "name", "esdhc");
+        qdev_prop_set_uint64(dev, "size", MPC85XX_ESDHC_REGS_SIZE);
+        s = SYS_BUS_DEVICE(dev);
+        sysbus_realize_and_unref(s, &error_fatal);
+        memory_region_add_subregion(ccsr_addr_space, MPC85XX_ESDHC_REGS_OFFSET,
+                                    sysbus_mmio_get_region(s, 0));
 
         /*
          * Compatible with:
-- 
2.39.1

Re: [PATCH 4/4] hw/ppc/e500.c: Attach eSDHC unimplemented region to ccsr_addr_space

[Philippe Mathieu-Daudé]

On 25/1/23 14:00, Bernhard Beschow wrote:
> Makes the unimplemented region move together with the CCSR address space
> if moved by a bootloader. Moving the CCSR address space isn't
> implemented yet but this patch is a preparation for it.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ppc/e500.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/4] hw/ppc: Set machine->fdt in e500 machines

[Daniel Henrique Barboza]

On 1/25/23 10:00, Bernhard Beschow wrote:
> This enables support for the 'dumpdtb' QMP/HMP command for all
> e500 machines.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ppc/e500.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 9fa1f8e6cf..7239993acc 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -659,9 +659,14 @@ done:
>       if (!dry_run) {
>           qemu_fdt_dumpdtb(fdt, fdt_size);
>           cpu_physical_memory_write(addr, fdt, fdt_size);
> +
> +        /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
> +        g_free(machine->fdt);
> +        machine->fdt = fdt;
> +    } else {
> +        g_free(fdt);
>       }
>       ret = fdt_size;
> -    g_free(fdt);
>   

I tried to do this change last year when introducing 'dumpdtb' and Phil had some
comments in how the FDT was being handled by the e500 board:

https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg03256.html


================

+
+    /*
+     * Update the machine->fdt pointer to enable support for the
+     * 'dumpdtb' QMP/HMP command.
+     *
+     * The FDT is re-created during reset,

Why are we doing that? Is it really necessary? This seems to be only required at cold power-on.

+ so free machine->fdt
+     * to avoid leaking the old FDT.
+     */
+    g_free(machine->fdt);
+    machine->fdt = fdt;
================

I ended up not going after Phil's concern. I don't think it's required to accept
this change, but it would simplify it a bit if the FDT isn't required to be
re-generated on boot.


I'm CCing Phil in case he wants to comment on it as well.




Daniel


>   out:
>       g_free(pci_map);

Re: [PATCH 1/4] hw/ppc: Set machine->fdt in e500 machines

[Bernhard Beschow]

Am 26. Januar 2023 15:58:18 UTC schrieb Daniel Henrique Barboza <danielhb413@gmail.com>:
>
>
>On 1/25/23 10:00, Bernhard Beschow wrote:
>> This enables support for the 'dumpdtb' QMP/HMP command for all
>> e500 machines.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/e500.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 9fa1f8e6cf..7239993acc 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -659,9 +659,14 @@ done:
>>       if (!dry_run) {
>>           qemu_fdt_dumpdtb(fdt, fdt_size);
>>           cpu_physical_memory_write(addr, fdt, fdt_size);
>> +
>> +        /* Set machine->fdt for 'dumpdtb' QMP/HMP command */
>> +        g_free(machine->fdt);
>> +        machine->fdt = fdt;
>> +    } else {
>> +        g_free(fdt);
>>       }
>>       ret = fdt_size;
>> -    g_free(fdt);
>>   
>
>I tried to do this change last year when introducing 'dumpdtb' and Phil had some
>comments in how the FDT was being handled by the e500 board:
>
>https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg03256.html
>
>
>================
>
>+
>+    /*
>+     * Update the machine->fdt pointer to enable support for the
>+     * 'dumpdtb' QMP/HMP command.
>+     *
>+     * The FDT is re-created during reset,
>
>Why are we doing that? Is it really necessary? This seems to be only required at cold power-on.

The e500 boards have user-creatable eTSEC nics which are registered with the machine via the platform bus mechanism. IIUC this mechanism causes these nics to show up only after reset. The nics need to show up in the device tree, so the reset trigger was apparently chosen to create the fdt.

N.B.: The size of the fdt needs to be known during machine_init to check whether it fits into guest ram. That's what the dry_run parameter is for.

Does that explanation help?

Best regards,
Bernhard

>
>+ so free machine->fdt
>+     * to avoid leaking the old FDT.
>+     */
>+    g_free(machine->fdt);
>+    machine->fdt = fdt;
>================
>
>I ended up not going after Phil's concern. I don't think it's required to accept
>this change, but it would simplify it a bit if the FDT isn't required to be
>re-generated on boot.
>
>
>I'm CCing Phil in case he wants to comment on it as well.
>
>
>
>
>Daniel
>
>
>>   out:
>>       g_free(pci_map);

Re: [PATCH 0/4] E500 cleanups and enhancements

[Daniel Henrique Barboza]

On 1/25/23 10:00, Bernhard Beschow wrote:
> This series includes some cleanups I came across when working on the ppce500
> machine. Furthermore, it enables support for the 'dumpdtb' QMP/HMP command
> which was missing so far.
> 
> Bernhard Beschow (4):
>    hw/ppc: Set machine->fdt in e500 machines
>    hw/ppc/e500{,plat}: Drop redundant checks for presence of platform bus
>    hw/ppc/e500.c: Avoid hardcoding parent device in
>      create_devtree_etsec()
>    hw/ppc/e500.c: Attach eSDHC unimplemented region to ccsr_addr_space

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

And queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

> 
>   hw/ppc/e500.c     | 24 ++++++++++++++++--------
>   hw/ppc/e500plat.c |  9 +++------
>   2 files changed, 19 insertions(+), 14 deletions(-)
>