[PATCH 00/23] E500 Cleanup

[PATCH 00/23] E500 Cleanup

[Bernhard Beschow]

This series is part of a bigger series exploring data-driven machine creation
using device tree blobs on top of the e500 machines [1]. It contains patches to
make this exploration easier which are also expected to provide value in
themselves.

The cleanup starts with the e500 machine class itself, then proceeds with
machine-specific device models and concludes with more or less loosely related
devices. Device cleanup mostly consists of using the DEFINE_TYPES() macro.

[1] https://github.com/shentok/qemu/tree/e500-fdt

Bernhard Beschow (23):
  hw/ppc/e500: Do not leak struct boot_info
  hw/ppc/e500: Reduce scope of env pointer
  hw/ppc/e500: Prefer QOM cast
  hw/ppc/e500: Remove unused "irqs" parameter
  hw/ppc/e500: Add missing device tree properties to i2c controller node
  hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal
    resources
  hw/ppc/e500: Extract ppce500_ccsr.c
  hw/ppc/ppce500_ccsr: Log access to unimplemented registers
  hw/ppc/mpc8544_guts: Populate POR PLL ratio status register
  hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access
  hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro
  hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define
  hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro
  hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro
  hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro
  hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro
  hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch
  hw/sd/sdhci: Prefer DEFINE_TYPES() macro
  hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro
  hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro
  hw/rtc/ds1338: Prefer DEFINE_TYPES() macro
  hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro
  hw/vfio/platform: Let vfio_start_eventfd_injection() take
    VFIOPlatformDevice pointer

 MAINTAINERS              |   2 +-
 hw/ppc/e500-ccsr.h       |   2 +
 hw/ppc/e500.h            |   8 +++
 hw/block/pflash_cfi01.c  |  21 +++----
 hw/gpio/mpc8xxx.c        |  22 +++-----
 hw/i2c/mpc_i2c.c         |  29 +++++-----
 hw/i2c/smbus_eeprom.c    |  19 +++----
 hw/net/fsl_etsec/etsec.c |  22 +++-----
 hw/pci-host/ppce500.c    |  54 ++++++++----------
 hw/ppc/e500.c            |  61 +++++---------------
 hw/ppc/mpc8544_guts.c    |  32 +++++++----
 hw/ppc/ppce500_ccsr.c    |  67 ++++++++++++++++++++++
 hw/rtc/ds1338.c          |  20 +++----
 hw/sd/sdhci.c            |  62 +++++++++-----------
 hw/usb/hcd-ehci-sysbus.c | 118 +++++++++++++++++----------------------
 hw/vfio/platform.c       |   7 +--
 hw/i2c/trace-events      |   5 ++
 hw/intc/Kconfig          |   4 ++
 hw/intc/meson.build      |   3 +-
 hw/ppc/meson.build       |   1 +
 hw/ppc/trace-events      |   3 +
 21 files changed, 285 insertions(+), 277 deletions(-)
 create mode 100644 hw/ppc/ppce500_ccsr.c

-- 
2.46.1

[PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info

[Bernhard Beschow]

The struct is allocated once with g_new0() but never free()'d. Fix the leakage
by adding an attribute to struct PPCE500MachineState which avoids the
allocation.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.h |  8 ++++++++
 hw/ppc/e500.c | 17 ++++-------------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 8c09ef92e4..557ce6ad93 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -5,10 +5,18 @@
 #include "hw/platform-bus.h"
 #include "qom/object.h"
 
+typedef struct boot_info {
+    uint32_t dt_base;
+    uint32_t dt_size;
+    uint32_t entry;
+} boot_info;
+
 struct PPCE500MachineState {
     /*< private >*/
     MachineState parent_obj;
 
+    boot_info boot_info;
+
     /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
      * board supports dynamic sysbus devices
      */
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 3bd12b54ab..75b051009f 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -80,13 +80,6 @@
 
 #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
 
-struct boot_info
-{
-    uint32_t dt_base;
-    uint32_t dt_size;
-    uint32_t entry;
-};
-
 static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
                                 int nr_slots, int *len)
 {
@@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
     bool kernel_as_payload;
     hwaddr bios_entry = 0;
     target_long payload_size;
-    struct boot_info *boot_info = NULL;
     int dt_size;
     int i;
     unsigned int smp_cpus = machine->smp.cpus;
@@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
         /* Register reset handler */
         if (!i) {
             /* Primary CPU */
-            boot_info = g_new0(struct boot_info, 1);
             qemu_register_reset(ppce500_cpu_reset, cpu);
-            env->load_info = boot_info;
+            env->load_info = &pms->boot_info;
         } else {
             /* Secondary CPUs */
             qemu_register_reset(ppce500_cpu_reset_sec, cpu);
@@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
     }
     assert(dt_size < DTB_MAX_SIZE);
 
-    boot_info->entry = bios_entry;
-    boot_info->dt_base = dt_base;
-    boot_info->dt_size = dt_size;
+    pms->boot_info.entry = bios_entry;
+    pms->boot_info.dt_base = dt_base;
+    pms->boot_info.dt_size = dt_size;
 }
 
 static void e500_ccsr_initfn(Object *obj)
-- 
2.46.1

Re: [PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> The struct is allocated once with g_new0() but never free()'d. Fix the leakage
> by adding an attribute to struct PPCE500MachineState which avoids the
> allocation.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/ppc/e500.h |  8 ++++++++
> hw/ppc/e500.c | 17 ++++-------------
> 2 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 8c09ef92e4..557ce6ad93 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -5,10 +5,18 @@
> #include "hw/platform-bus.h"
> #include "qom/object.h"
>
> +typedef struct boot_info {
> +    uint32_t dt_base;
> +    uint32_t dt_size;
> +    uint32_t entry;
> +} boot_info;
> +
> struct PPCE500MachineState {
>     /*< private >*/

While at it you could remove these private markers...

>     MachineState parent_obj;
>
> +    boot_info boot_info;
> +

...and drop the new line here so only the parent_obj is followed by a new 
line as was suggested as reccomended style.

Regatds,
BALATON Zoltan

>     /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
>      * board supports dynamic sysbus devices
>      */
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 3bd12b54ab..75b051009f 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -80,13 +80,6 @@
>
> #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
>
> -struct boot_info
> -{
> -    uint32_t dt_base;
> -    uint32_t dt_size;
> -    uint32_t entry;
> -};
> -
> static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>                                 int nr_slots, int *len)
> {
> @@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
>     bool kernel_as_payload;
>     hwaddr bios_entry = 0;
>     target_long payload_size;
> -    struct boot_info *boot_info = NULL;
>     int dt_size;
>     int i;
>     unsigned int smp_cpus = machine->smp.cpus;
> @@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
>         /* Register reset handler */
>         if (!i) {
>             /* Primary CPU */
> -            boot_info = g_new0(struct boot_info, 1);
>             qemu_register_reset(ppce500_cpu_reset, cpu);
> -            env->load_info = boot_info;
> +            env->load_info = &pms->boot_info;
>         } else {
>             /* Secondary CPUs */
>             qemu_register_reset(ppce500_cpu_reset_sec, cpu);
> @@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
>     }
>     assert(dt_size < DTB_MAX_SIZE);
>
> -    boot_info->entry = bios_entry;
> -    boot_info->dt_base = dt_base;
> -    boot_info->dt_size = dt_size;
> +    pms->boot_info.entry = bios_entry;
> +    pms->boot_info.dt_base = dt_base;
> +    pms->boot_info.dt_size = dt_size;
> }
>
> static void e500_ccsr_initfn(Object *obj)
>

Re: [PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info

[Bernhard Beschow]

Am 23. September 2024 10:02:10 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> The struct is allocated once with g_new0() but never free()'d. Fix the leakage
>> by adding an attribute to struct PPCE500MachineState which avoids the
>> allocation.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/e500.h |  8 ++++++++
>> hw/ppc/e500.c | 17 ++++-------------
>> 2 files changed, 12 insertions(+), 13 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
>> index 8c09ef92e4..557ce6ad93 100644
>> --- a/hw/ppc/e500.h
>> +++ b/hw/ppc/e500.h
>> @@ -5,10 +5,18 @@
>> #include "hw/platform-bus.h"
>> #include "qom/object.h"
>> 
>> +typedef struct boot_info {
>> +    uint32_t dt_base;
>> +    uint32_t dt_size;
>> +    uint32_t entry;
>> +} boot_info;
>> +
>> struct PPCE500MachineState {
>>     /*< private >*/
>
>While at it you could remove these private markers...

Will do.

>
>>     MachineState parent_obj;
>> 
>> +    boot_info boot_info;
>> +
>
>...and drop the new line here so only the parent_obj is followed by a new line as was suggested as reccomended style.

I'll merge struct boot_info below as Cédric suggested.

Best regards,
Bernhard

>
>Regatds,
>BALATON Zoltan
>
>>     /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
>>      * board supports dynamic sysbus devices
>>      */
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 3bd12b54ab..75b051009f 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -80,13 +80,6 @@
>> 
>> #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
>> 
>> -struct boot_info
>> -{
>> -    uint32_t dt_base;
>> -    uint32_t dt_size;
>> -    uint32_t entry;
>> -};
>> -
>> static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>>                                 int nr_slots, int *len)
>> {
>> @@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
>>     bool kernel_as_payload;
>>     hwaddr bios_entry = 0;
>>     target_long payload_size;
>> -    struct boot_info *boot_info = NULL;
>>     int dt_size;
>>     int i;
>>     unsigned int smp_cpus = machine->smp.cpus;
>> @@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
>>         /* Register reset handler */
>>         if (!i) {
>>             /* Primary CPU */
>> -            boot_info = g_new0(struct boot_info, 1);
>>             qemu_register_reset(ppce500_cpu_reset, cpu);
>> -            env->load_info = boot_info;
>> +            env->load_info = &pms->boot_info;
>>         } else {
>>             /* Secondary CPUs */
>>             qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>> @@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
>>     }
>>     assert(dt_size < DTB_MAX_SIZE);
>> 
>> -    boot_info->entry = bios_entry;
>> -    boot_info->dt_base = dt_base;
>> -    boot_info->dt_size = dt_size;
>> +    pms->boot_info.entry = bios_entry;
>> +    pms->boot_info.dt_base = dt_base;
>> +    pms->boot_info.dt_size = dt_size;
>> }
>> 
>> static void e500_ccsr_initfn(Object *obj)
>> 

Re: [PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info

[Cédric Le Goater]

On 9/23/24 11:29, Bernhard Beschow wrote:
> The struct is allocated once with g_new0() but never free()'d. Fix the leakage
> by adding an attribute to struct PPCE500MachineState which avoids the
> allocation.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ppc/e500.h |  8 ++++++++
>   hw/ppc/e500.c | 17 ++++-------------
>   2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 8c09ef92e4..557ce6ad93 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -5,10 +5,18 @@
>   #include "hw/platform-bus.h"
>   #include "qom/object.h"
>   
> +typedef struct boot_info {
> +    uint32_t dt_base;
> +    uint32_t dt_size;
> +    uint32_t entry;
> +} boot_info;

or simply move the fields under the machine state struct to avoif
the struct boot_info which doesn't seem that necessary. Is it ?


Thanks,

C.



> +
>   struct PPCE500MachineState {
>       /*< private >*/
>       MachineState parent_obj;
>   
> +    boot_info boot_info;
> +
>       /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
>        * board supports dynamic sysbus devices
>        */
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 3bd12b54ab..75b051009f 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -80,13 +80,6 @@
>   
>   #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
>   
> -struct boot_info
> -{
> -    uint32_t dt_base;
> -    uint32_t dt_size;
> -    uint32_t entry;
> -};
> -
>   static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>                                   int nr_slots, int *len)
>   {
> @@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
>       bool kernel_as_payload;
>       hwaddr bios_entry = 0;
>       target_long payload_size;
> -    struct boot_info *boot_info = NULL;
>       int dt_size;
>       int i;
>       unsigned int smp_cpus = machine->smp.cpus;
> @@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
>           /* Register reset handler */
>           if (!i) {
>               /* Primary CPU */
> -            boot_info = g_new0(struct boot_info, 1);
>               qemu_register_reset(ppce500_cpu_reset, cpu);
> -            env->load_info = boot_info;
> +            env->load_info = &pms->boot_info;
>           } else {
>               /* Secondary CPUs */
>               qemu_register_reset(ppce500_cpu_reset_sec, cpu);
> @@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
>       }
>       assert(dt_size < DTB_MAX_SIZE);
>   
> -    boot_info->entry = bios_entry;
> -    boot_info->dt_base = dt_base;
> -    boot_info->dt_size = dt_size;
> +    pms->boot_info.entry = bios_entry;
> +    pms->boot_info.dt_base = dt_base;
> +    pms->boot_info.dt_size = dt_size;
>   }
>   
>   static void e500_ccsr_initfn(Object *obj)

Re: [PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info

[Bernhard Beschow]

Am 25. September 2024 15:35:15 UTC schrieb "Cédric Le Goater" <clg@redhat.com>:
>On 9/23/24 11:29, Bernhard Beschow wrote:
>> The struct is allocated once with g_new0() but never free()'d. Fix the leakage
>> by adding an attribute to struct PPCE500MachineState which avoids the
>> allocation.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/e500.h |  8 ++++++++
>>   hw/ppc/e500.c | 17 ++++-------------
>>   2 files changed, 12 insertions(+), 13 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
>> index 8c09ef92e4..557ce6ad93 100644
>> --- a/hw/ppc/e500.h
>> +++ b/hw/ppc/e500.h
>> @@ -5,10 +5,18 @@
>>   #include "hw/platform-bus.h"
>>   #include "qom/object.h"
>>   +typedef struct boot_info {
>> +    uint32_t dt_base;
>> +    uint32_t dt_size;
>> +    uint32_t entry;
>> +} boot_info;
>
>or simply move the fields under the machine state struct to avoif
>the struct boot_info which doesn't seem that necessary. Is it ?

Yes, this works. Good idea.

Best regards,
Bernhard

>
>
>Thanks,
>
>C.
>
>
>
>> +
>>   struct PPCE500MachineState {
>>       /*< private >*/
>>       MachineState parent_obj;
>>   +    boot_info boot_info;
>> +
>>       /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
>>        * board supports dynamic sysbus devices
>>        */
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 3bd12b54ab..75b051009f 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -80,13 +80,6 @@
>>     #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
>>   -struct boot_info
>> -{
>> -    uint32_t dt_base;
>> -    uint32_t dt_size;
>> -    uint32_t entry;
>> -};
>> -
>>   static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>>                                   int nr_slots, int *len)
>>   {
>> @@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
>>       bool kernel_as_payload;
>>       hwaddr bios_entry = 0;
>>       target_long payload_size;
>> -    struct boot_info *boot_info = NULL;
>>       int dt_size;
>>       int i;
>>       unsigned int smp_cpus = machine->smp.cpus;
>> @@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
>>           /* Register reset handler */
>>           if (!i) {
>>               /* Primary CPU */
>> -            boot_info = g_new0(struct boot_info, 1);
>>               qemu_register_reset(ppce500_cpu_reset, cpu);
>> -            env->load_info = boot_info;
>> +            env->load_info = &pms->boot_info;
>>           } else {
>>               /* Secondary CPUs */
>>               qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>> @@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
>>       }
>>       assert(dt_size < DTB_MAX_SIZE);
>>   -    boot_info->entry = bios_entry;
>> -    boot_info->dt_base = dt_base;
>> -    boot_info->dt_size = dt_size;
>> +    pms->boot_info.entry = bios_entry;
>> +    pms->boot_info.dt_base = dt_base;
>> +    pms->boot_info.dt_size = dt_size;
>>   }
>>     static void e500_ccsr_initfn(Object *obj)
>

Re: [PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3384 bytes --]

On Wed, 25 Sep 2024, Cédric Le Goater wrote:
> On 9/23/24 11:29, Bernhard Beschow wrote:
>> The struct is allocated once with g_new0() but never free()'d. Fix the 
>> leakage
>> by adding an attribute to struct PPCE500MachineState which avoids the
>> allocation.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/e500.h |  8 ++++++++
>>   hw/ppc/e500.c | 17 ++++-------------
>>   2 files changed, 12 insertions(+), 13 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
>> index 8c09ef92e4..557ce6ad93 100644
>> --- a/hw/ppc/e500.h
>> +++ b/hw/ppc/e500.h
>> @@ -5,10 +5,18 @@
>>   #include "hw/platform-bus.h"
>>   #include "qom/object.h"
>>   +typedef struct boot_info {
>> +    uint32_t dt_base;
>> +    uint32_t dt_size;
>> +    uint32_t entry;
>> +} boot_info;
>
> or simply move the fields under the machine state struct to avoif
> the struct boot_info which doesn't seem that necessary. Is it ?

It's passed to CPU reset function via env->load_info. It could be possible 
to pass the whole machine state but it seems that's unneeded so this 
struct just contains what's needed for this. Other machines also have 
similar boot_info structs although they seem to be different and not 
common to all machines. Thus I don't think merging with machine state 
would be better than keeping is separate as this is more CPU related.

Regards,
BALATON Zoltan

>
> Thanks,
>
> C.
>
>
>
>> +
>>   struct PPCE500MachineState {
>>       /*< private >*/
>>       MachineState parent_obj;
>>   +    boot_info boot_info;
>> +
>>       /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
>>        * board supports dynamic sysbus devices
>>        */
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 3bd12b54ab..75b051009f 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -80,13 +80,6 @@
>>     #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
>>   -struct boot_info
>> -{
>> -    uint32_t dt_base;
>> -    uint32_t dt_size;
>> -    uint32_t entry;
>> -};
>> -
>>   static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>>                                   int nr_slots, int *len)
>>   {
>> @@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
>>       bool kernel_as_payload;
>>       hwaddr bios_entry = 0;
>>       target_long payload_size;
>> -    struct boot_info *boot_info = NULL;
>>       int dt_size;
>>       int i;
>>       unsigned int smp_cpus = machine->smp.cpus;
>> @@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
>>           /* Register reset handler */
>>           if (!i) {
>>               /* Primary CPU */
>> -            boot_info = g_new0(struct boot_info, 1);
>>               qemu_register_reset(ppce500_cpu_reset, cpu);
>> -            env->load_info = boot_info;
>> +            env->load_info = &pms->boot_info;
>>           } else {
>>               /* Secondary CPUs */
>>               qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>> @@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
>>       }
>>       assert(dt_size < DTB_MAX_SIZE);
>>   -    boot_info->entry = bios_entry;
>> -    boot_info->dt_base = dt_base;
>> -    boot_info->dt_size = dt_size;
>> +    pms->boot_info.entry = bios_entry;
>> +    pms->boot_info.dt_base = dt_base;
>> +    pms->boot_info.dt_size = dt_size;
>>   }
>>     static void e500_ccsr_initfn(Object *obj)
>
>
>

Re: [PATCH 01/23] hw/ppc/e500: Do not leak struct boot_info

[Bernhard Beschow]

Am 26. September 2024 00:14:04 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 25 Sep 2024, Cédric Le Goater wrote:
>> On 9/23/24 11:29, Bernhard Beschow wrote:
>>> The struct is allocated once with g_new0() but never free()'d. Fix the leakage
>>> by adding an attribute to struct PPCE500MachineState which avoids the
>>> allocation.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   hw/ppc/e500.h |  8 ++++++++
>>>   hw/ppc/e500.c | 17 ++++-------------
>>>   2 files changed, 12 insertions(+), 13 deletions(-)
>>> 
>>> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
>>> index 8c09ef92e4..557ce6ad93 100644
>>> --- a/hw/ppc/e500.h
>>> +++ b/hw/ppc/e500.h
>>> @@ -5,10 +5,18 @@
>>>   #include "hw/platform-bus.h"
>>>   #include "qom/object.h"
>>>   +typedef struct boot_info {
>>> +    uint32_t dt_base;
>>> +    uint32_t dt_size;
>>> +    uint32_t entry;
>>> +} boot_info;
>> 
>> or simply move the fields under the machine state struct to avoif
>> the struct boot_info which doesn't seem that necessary. Is it ?
>
>It's passed to CPU reset function via env->load_info. It could be possible to pass the whole machine state but it seems that's unneeded so this struct just contains what's needed for this. Other machines also have similar boot_info structs although they seem to be different and not common to all machines. Thus I don't think merging with machine state would be better than keeping is separate as this is more CPU related.

I agree that having a consistent style across machines is prefereable, so I'll keep struct boot_info. It also avoids conflicts with your TLB cleanup series.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> 
>> Thanks,
>> 
>> C.
>> 
>> 
>> 
>>> +
>>>   struct PPCE500MachineState {
>>>       /*< private >*/
>>>       MachineState parent_obj;
>>>   +    boot_info boot_info;
>>> +
>>>       /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
>>>        * board supports dynamic sysbus devices
>>>        */
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index 3bd12b54ab..75b051009f 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -80,13 +80,6 @@
>>>     #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
>>>   -struct boot_info
>>> -{
>>> -    uint32_t dt_base;
>>> -    uint32_t dt_size;
>>> -    uint32_t entry;
>>> -};
>>> -
>>>   static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>>>                                   int nr_slots, int *len)
>>>   {
>>> @@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
>>>       bool kernel_as_payload;
>>>       hwaddr bios_entry = 0;
>>>       target_long payload_size;
>>> -    struct boot_info *boot_info = NULL;
>>>       int dt_size;
>>>       int i;
>>>       unsigned int smp_cpus = machine->smp.cpus;
>>> @@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
>>>           /* Register reset handler */
>>>           if (!i) {
>>>               /* Primary CPU */
>>> -            boot_info = g_new0(struct boot_info, 1);
>>>               qemu_register_reset(ppce500_cpu_reset, cpu);
>>> -            env->load_info = boot_info;
>>> +            env->load_info = &pms->boot_info;
>>>           } else {
>>>               /* Secondary CPUs */
>>>               qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>>> @@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
>>>       }
>>>       assert(dt_size < DTB_MAX_SIZE);
>>>   -    boot_info->entry = bios_entry;
>>> -    boot_info->dt_base = dt_base;
>>> -    boot_info->dt_size = dt_size;
>>> +    pms->boot_info.entry = bios_entry;
>>> +    pms->boot_info.dt_base = dt_base;
>>> +    pms->boot_info.dt_size = dt_size;
>>>   }
>>>     static void e500_ccsr_initfn(Object *obj)
>> 
>> 
>>

[PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer

[Bernhard Beschow]

The env pointer isn't used outside the for loop, so move it inside. After that,
the firstenv pointer is never read, so remove it.

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

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 75b051009f..f68779a1ea 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
     const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
     MachineClass *mc = MACHINE_CLASS(pmc);
     PCIBus *pci_bus;
-    CPUPPCState *env = NULL;
     uint64_t loadaddr;
     hwaddr kernel_base = -1LL;
     int kernel_size = 0;
@@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
     IrqLines *irqs;
     DeviceState *dev, *mpicdev;
     DriveInfo *dinfo;
-    CPUPPCState *firstenv = NULL;
     MemoryRegion *ccsr_addr_space;
     SysBusDevice *s;
     PPCE500CCSRState *ccsr;
@@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
     irqs = g_new0(IrqLines, smp_cpus);
     for (i = 0; i < smp_cpus; i++) {
         PowerPCCPU *cpu;
+        CPUPPCState *env;
         CPUState *cs;
 
         cpu = POWERPC_CPU(object_new(machine->cpu_type));
@@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
                                  &error_abort);
         qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
 
-        if (!firstenv) {
-            firstenv = env;
-        }
-
         irqs[i].irq[OPENPIC_OUTPUT_INT] =
             qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
         irqs[i].irq[OPENPIC_OUTPUT_CINT] =
@@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
         }
     }
 
-    env = firstenv;
-
     if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
         error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
         exit(EXIT_FAILURE);
-- 
2.46.1

Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> The env pointer isn't used outside the for loop, so move it inside. After that,
> the firstenv pointer is never read, so remove it.

It's probably the other way arouns, you remove firstenv (which is the 
bigger part of this patch) then it's clear env is not needed outside of 
the loop any more so can be moved there. The purpose of this seems to be 
to preserve the env of the first CPU but as it's unused yet maybe it can 
be removed for now and readded later when needed.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/ppc/e500.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 75b051009f..f68779a1ea 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>     const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>     MachineClass *mc = MACHINE_CLASS(pmc);
>     PCIBus *pci_bus;
> -    CPUPPCState *env = NULL;
>     uint64_t loadaddr;
>     hwaddr kernel_base = -1LL;
>     int kernel_size = 0;
> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>     IrqLines *irqs;
>     DeviceState *dev, *mpicdev;
>     DriveInfo *dinfo;
> -    CPUPPCState *firstenv = NULL;
>     MemoryRegion *ccsr_addr_space;
>     SysBusDevice *s;
>     PPCE500CCSRState *ccsr;
> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>     irqs = g_new0(IrqLines, smp_cpus);
>     for (i = 0; i < smp_cpus; i++) {
>         PowerPCCPU *cpu;
> +        CPUPPCState *env;
>         CPUState *cs;
>
>         cpu = POWERPC_CPU(object_new(machine->cpu_type));
> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>                                  &error_abort);
>         qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>
> -        if (!firstenv) {
> -            firstenv = env;
> -        }
> -
>         irqs[i].irq[OPENPIC_OUTPUT_INT] =
>             qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>         irqs[i].irq[OPENPIC_OUTPUT_CINT] =
> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>         }
>     }
>
> -    env = firstenv;
> -
>     if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>         error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>         exit(EXIT_FAILURE);
>

Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer

[Bernhard Beschow]

Am 23. September 2024 10:04:48 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> The env pointer isn't used outside the for loop, so move it inside. After that,
>> the firstenv pointer is never read, so remove it.
>
>It's probably the other way arouns, you remove firstenv (which is the bigger part of this patch) then it's clear env is not needed outside of the loop any more so can be moved there. The purpose of this seems to be to preserve the env of the first CPU but as it's unused yet maybe it can be removed for now and readded later when needed.

I'll fix the commit message.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/e500.c | 9 +--------
>> 1 file changed, 1 insertion(+), 8 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 75b051009f..f68779a1ea 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>>     const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>>     MachineClass *mc = MACHINE_CLASS(pmc);
>>     PCIBus *pci_bus;
>> -    CPUPPCState *env = NULL;
>>     uint64_t loadaddr;
>>     hwaddr kernel_base = -1LL;
>>     int kernel_size = 0;
>> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>>     IrqLines *irqs;
>>     DeviceState *dev, *mpicdev;
>>     DriveInfo *dinfo;
>> -    CPUPPCState *firstenv = NULL;
>>     MemoryRegion *ccsr_addr_space;
>>     SysBusDevice *s;
>>     PPCE500CCSRState *ccsr;
>> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>>     irqs = g_new0(IrqLines, smp_cpus);
>>     for (i = 0; i < smp_cpus; i++) {
>>         PowerPCCPU *cpu;
>> +        CPUPPCState *env;
>>         CPUState *cs;
>> 
>>         cpu = POWERPC_CPU(object_new(machine->cpu_type));
>> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>>                                  &error_abort);
>>         qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>> 
>> -        if (!firstenv) {
>> -            firstenv = env;
>> -        }
>> -
>>         irqs[i].irq[OPENPIC_OUTPUT_INT] =
>>             qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>>         irqs[i].irq[OPENPIC_OUTPUT_CINT] =
>> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>>         }
>>     }
>> 
>> -    env = firstenv;
>> -
>>     if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>>         error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>>         exit(EXIT_FAILURE);
>> 

Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer

[Cédric Le Goater]

On 9/23/24 11:29, Bernhard Beschow wrote:
> The env pointer isn't used outside the for loop, so move it inside. After that,
> the firstenv pointer is never read, so remove it.

Just wondering, have you considered introducing an PowerPCCPU array
under the machine state ?

This would be an intermediate step towards the introduction of an SoC
model (in the long term)

Thanks,

C.



> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ppc/e500.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 75b051009f..f68779a1ea 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>       const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>       MachineClass *mc = MACHINE_CLASS(pmc);
>       PCIBus *pci_bus;
> -    CPUPPCState *env = NULL;
>       uint64_t loadaddr;
>       hwaddr kernel_base = -1LL;
>       int kernel_size = 0;
> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>       IrqLines *irqs;
>       DeviceState *dev, *mpicdev;
>       DriveInfo *dinfo;
> -    CPUPPCState *firstenv = NULL;
>       MemoryRegion *ccsr_addr_space;
>       SysBusDevice *s;
>       PPCE500CCSRState *ccsr;
> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>       irqs = g_new0(IrqLines, smp_cpus);
>       for (i = 0; i < smp_cpus; i++) {
>           PowerPCCPU *cpu;
> +        CPUPPCState *env;
>           CPUState *cs;
>   
>           cpu = POWERPC_CPU(object_new(machine->cpu_type));
> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>                                    &error_abort);
>           qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>   
> -        if (!firstenv) {
> -            firstenv = env;
> -        }
> -
>           irqs[i].irq[OPENPIC_OUTPUT_INT] =
>               qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>           irqs[i].irq[OPENPIC_OUTPUT_CINT] =
> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>           }
>       }
>   
> -    env = firstenv;
> -
>       if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>           error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>           exit(EXIT_FAILURE);

Re: [PATCH 02/23] hw/ppc/e500: Reduce scope of env pointer

[Bernhard Beschow]

Am 25. September 2024 15:37:22 UTC schrieb "Cédric Le Goater" <clg@redhat.com>:
>On 9/23/24 11:29, Bernhard Beschow wrote:
>> The env pointer isn't used outside the for loop, so move it inside. After that,
>> the firstenv pointer is never read, so remove it.
>
>Just wondering, have you considered introducing an PowerPCCPU array
>under the machine state ?
>
>This would be an intermediate step towards the introduction of an SoC
>model (in the long term)

Well, there seem to be many members in the QorIQ family with incompatible offsets. So I experimented with dtb-driven machine creation instead to sidestep the whole problem. Once this series is merged I plan to submit an RFC for that.

Best regards,
Bernhard

>
>Thanks,
>
>C.
>
>
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/e500.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 75b051009f..f68779a1ea 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -899,7 +899,6 @@ void ppce500_init(MachineState *machine)
>>       const PPCE500MachineClass *pmc = PPCE500_MACHINE_GET_CLASS(machine);
>>       MachineClass *mc = MACHINE_CLASS(pmc);
>>       PCIBus *pci_bus;
>> -    CPUPPCState *env = NULL;
>>       uint64_t loadaddr;
>>       hwaddr kernel_base = -1LL;
>>       int kernel_size = 0;
>> @@ -921,7 +920,6 @@ void ppce500_init(MachineState *machine)
>>       IrqLines *irqs;
>>       DeviceState *dev, *mpicdev;
>>       DriveInfo *dinfo;
>> -    CPUPPCState *firstenv = NULL;
>>       MemoryRegion *ccsr_addr_space;
>>       SysBusDevice *s;
>>       PPCE500CCSRState *ccsr;
>> @@ -930,6 +928,7 @@ void ppce500_init(MachineState *machine)
>>       irqs = g_new0(IrqLines, smp_cpus);
>>       for (i = 0; i < smp_cpus; i++) {
>>           PowerPCCPU *cpu;
>> +        CPUPPCState *env;
>>           CPUState *cs;
>>             cpu = POWERPC_CPU(object_new(machine->cpu_type));
>> @@ -950,10 +949,6 @@ void ppce500_init(MachineState *machine)
>>                                    &error_abort);
>>           qdev_realize_and_unref(DEVICE(cs), NULL, &error_fatal);
>>   -        if (!firstenv) {
>> -            firstenv = env;
>> -        }
>> -
>>           irqs[i].irq[OPENPIC_OUTPUT_INT] =
>>               qdev_get_gpio_in(DEVICE(cpu), PPCE500_INPUT_INT);
>>           irqs[i].irq[OPENPIC_OUTPUT_CINT] =
>> @@ -974,8 +969,6 @@ void ppce500_init(MachineState *machine)
>>           }
>>       }
>>   -    env = firstenv;
>> -
>>       if (!QEMU_IS_ALIGNED(machine->ram_size, RAM_SIZES_ALIGN)) {
>>           error_report("RAM size must be multiple of %" PRIu64, RAM_SIZES_ALIGN);
>>           exit(EXIT_FAILURE);
>

[PATCH 03/23] hw/ppc/e500: Prefer QOM cast

[Bernhard Beschow]

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

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index f68779a1ea..32996c188e 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1008,7 +1008,7 @@ void ppce500_init(MachineState *machine)
     sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC8544_I2C_IRQ));
     memory_region_add_subregion(ccsr_addr_space, MPC8544_I2C_REGS_OFFSET,
                                 sysbus_mmio_get_region(s, 0));
-    i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
+    i2c = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
     i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
 
     /* eSDHC */
@@ -1057,7 +1057,7 @@ void ppce500_init(MachineState *machine)
     memory_region_add_subregion(ccsr_addr_space, MPC8544_PCI_REGS_OFFSET,
                                 sysbus_mmio_get_region(s, 0));
 
-    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
+    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
     if (!pci_bus)
         printf("couldn't create PCI controller!\n");
 
-- 
2.46.1

Re: [PATCH 03/23] hw/ppc/e500: Prefer QOM cast

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/ppc/e500.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index f68779a1ea..32996c188e 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -1008,7 +1008,7 @@ void ppce500_init(MachineState *machine)
>     sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC8544_I2C_IRQ));
>     memory_region_add_subregion(ccsr_addr_space, MPC8544_I2C_REGS_OFFSET,
>                                 sysbus_mmio_get_region(s, 0));
> -    i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
> +    i2c = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
>     i2c_slave_create_simple(i2c, "ds1338", RTC_REGS_OFFSET);
>
>     /* eSDHC */
> @@ -1057,7 +1057,7 @@ void ppce500_init(MachineState *machine)
>     memory_region_add_subregion(ccsr_addr_space, MPC8544_PCI_REGS_OFFSET,
>                                 sysbus_mmio_get_region(s, 0));
>
> -    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
> +    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
>     if (!pci_bus)
>         printf("couldn't create PCI controller!\n");
>
>

[PATCH 04/23] hw/ppc/e500: Remove unused "irqs" parameter

[Bernhard Beschow]

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

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 32996c188e..228287b457 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -825,7 +825,7 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500MachineState *pms,
 }
 
 static DeviceState *ppce500_init_mpic_kvm(const PPCE500MachineClass *pmc,
-                                          IrqLines *irqs, Error **errp)
+                                          Error **errp)
 {
 #ifdef CONFIG_KVM
     DeviceState *dev;
@@ -865,7 +865,7 @@ static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
         Error *err = NULL;
 
         if (kvm_kernel_irqchip_allowed()) {
-            dev = ppce500_init_mpic_kvm(pmc, irqs, &err);
+            dev = ppce500_init_mpic_kvm(pmc, &err);
         }
         if (kvm_kernel_irqchip_required() && !dev) {
             error_reportf_err(err,
-- 
2.46.1

Re: [PATCH 04/23] hw/ppc/e500: Remove unused "irqs" parameter

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/ppc/e500.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 32996c188e..228287b457 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -825,7 +825,7 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500MachineState *pms,
> }
>
> static DeviceState *ppce500_init_mpic_kvm(const PPCE500MachineClass *pmc,
> -                                          IrqLines *irqs, Error **errp)
> +                                          Error **errp)
> {
> #ifdef CONFIG_KVM
>     DeviceState *dev;
> @@ -865,7 +865,7 @@ static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
>         Error *err = NULL;
>
>         if (kvm_kernel_irqchip_allowed()) {
> -            dev = ppce500_init_mpic_kvm(pmc, irqs, &err);
> +            dev = ppce500_init_mpic_kvm(pmc, &err);
>         }
>         if (kvm_kernel_irqchip_required() && !dev) {
>             error_reportf_err(err,
>

[PATCH 05/23] hw/ppc/e500: Add missing device tree properties to i2c controller node

[Bernhard Beschow]

When compiling a decompiled device tree blob created with dumpdtb, dtc complains
with:

  /soc@e0000000/i2c@3000: incorrect #address-cells for I2C bus
  /soc@e0000000/i2c@3000: incorrect #size-cells for I2C bus

Fix this by adding the missing device tree properties.

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

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 228287b457..e2a4f265a5 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -196,6 +196,8 @@ static void dt_i2c_create(void *fdt, const char *soc, const char *mpic,
     qemu_fdt_setprop_cells(fdt, i2c, "cell-index", 0);
     qemu_fdt_setprop_cells(fdt, i2c, "interrupts", irq0, 0x2);
     qemu_fdt_setprop_phandle(fdt, i2c, "interrupt-parent", mpic);
+    qemu_fdt_setprop_cell(fdt, i2c, "#size-cells", 0);
+    qemu_fdt_setprop_cell(fdt, i2c, "#address-cells", 1);
     qemu_fdt_setprop_string(fdt, "/aliases", alias, i2c);
 
     g_free(i2c);
-- 
2.46.1

Re: [PATCH 05/23] hw/ppc/e500: Add missing device tree properties to i2c controller node

[Cédric Le Goater]

On 9/23/24 11:29, Bernhard Beschow wrote:
> When compiling a decompiled device tree blob created with dumpdtb, dtc complains
> with:
> 
>    /soc@e0000000/i2c@3000: incorrect #address-cells for I2C bus
>    /soc@e0000000/i2c@3000: incorrect #size-cells for I2C bus
> 
> Fix this by adding the missing device tree properties.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/ppc/e500.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 228287b457..e2a4f265a5 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -196,6 +196,8 @@ static void dt_i2c_create(void *fdt, const char *soc, const char *mpic,
>       qemu_fdt_setprop_cells(fdt, i2c, "cell-index", 0);
>       qemu_fdt_setprop_cells(fdt, i2c, "interrupts", irq0, 0x2);
>       qemu_fdt_setprop_phandle(fdt, i2c, "interrupt-parent", mpic);
> +    qemu_fdt_setprop_cell(fdt, i2c, "#size-cells", 0);
> +    qemu_fdt_setprop_cell(fdt, i2c, "#address-cells", 1);
>       qemu_fdt_setprop_string(fdt, "/aliases", alias, i2c);
>   
>       g_free(i2c);

[PATCH 06/23] hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal resources

[Bernhard Beschow]

Rather than accessing the attributes of TYPE_CCSR directly, use the SysBusDevice
API which exists exactly for that purpose. Furthermore, registering the memory
region with the SysBusDevice API makes it show up in QMP's `info qom-tree`
command.

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

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index 95b983b2b3..97e5d47cec 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -16,7 +16,6 @@
 
 #include "qemu/osdep.h"
 #include "hw/irq.h"
-#include "hw/ppc/e500-ccsr.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "hw/pci/pci_device.h"
@@ -419,11 +418,12 @@ static const VMStateDescription vmstate_ppce500_pci = {
 static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
 {
     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
-    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
-                                  "/e500-ccsr"));
+    SysBusDevice *ccsr = SYS_BUS_DEVICE(container_get(qdev_get_machine(),
+                                                      "/e500-ccsr"));
+    MemoryRegion *ccsr_space = sysbus_mmio_get_region(ccsr, 0);
 
-    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
-                             0, int128_get64(ccsr->ccsr_space.size));
+    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0",
+                             ccsr_space, 0, int128_get64(ccsr_space->size));
     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
 }
 
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index e2a4f265a5..2225533e33 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -924,7 +924,6 @@ void ppce500_init(MachineState *machine)
     DriveInfo *dinfo;
     MemoryRegion *ccsr_addr_space;
     SysBusDevice *s;
-    PPCE500CCSRState *ccsr;
     I2CBus *i2c;
 
     irqs = g_new0(IrqLines, smp_cpus);
@@ -980,10 +979,10 @@ void ppce500_init(MachineState *machine)
     memory_region_add_subregion(address_space_mem, 0, machine->ram);
 
     dev = qdev_new("e500-ccsr");
+    s = SYS_BUS_DEVICE(dev);
     object_property_add_child(OBJECT(machine), "e500-ccsr", OBJECT(dev));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    ccsr = CCSR(dev);
-    ccsr_addr_space = &ccsr->ccsr_space;
+    sysbus_realize_and_unref(s, &error_fatal);
+    ccsr_addr_space = sysbus_mmio_get_region(s, 0);
     memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
                                 ccsr_addr_space);
 
@@ -1270,6 +1269,7 @@ static void e500_ccsr_initfn(Object *obj)
     PPCE500CCSRState *ccsr = CCSR(obj);
     memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
                        MPC8544_CCSRBAR_SIZE);
+    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
 }
 
 static const TypeInfo e500_ccsr_info = {
-- 
2.46.1

Re: [PATCH 06/23] hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal resources

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Rather than accessing the attributes of TYPE_CCSR directly, use the SysBusDevice
> API which exists exactly for that purpose. Furthermore, registering the memory
> region with the SysBusDevice API makes it show up in QMP's `info qom-tree`
> command.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/pci-host/ppce500.c | 10 +++++-----
> hw/ppc/e500.c         |  8 ++++----
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index 95b983b2b3..97e5d47cec 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -16,7 +16,6 @@
>
> #include "qemu/osdep.h"
> #include "hw/irq.h"
> -#include "hw/ppc/e500-ccsr.h"
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> #include "hw/pci/pci_device.h"
> @@ -419,11 +418,12 @@ static const VMStateDescription vmstate_ppce500_pci = {
> static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
> {
>     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
> -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
> -                                  "/e500-ccsr"));
> +    SysBusDevice *ccsr = SYS_BUS_DEVICE(container_get(qdev_get_machine(),
> +                                                      "/e500-ccsr"));
> +    MemoryRegion *ccsr_space = sysbus_mmio_get_region(ccsr, 0);
>
> -    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
> -                             0, int128_get64(ccsr->ccsr_space.size));
> +    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0",
> +                             ccsr_space, 0, int128_get64(ccsr_space->size));

I wonder if this really needs an alias or could the original memory region 
be registered as the PCI BAR region? Otherwise:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Regards,
BALATON Zoltan

>     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
> }
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e2a4f265a5..2225533e33 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -924,7 +924,6 @@ void ppce500_init(MachineState *machine)
>     DriveInfo *dinfo;
>     MemoryRegion *ccsr_addr_space;
>     SysBusDevice *s;
> -    PPCE500CCSRState *ccsr;
>     I2CBus *i2c;
>
>     irqs = g_new0(IrqLines, smp_cpus);
> @@ -980,10 +979,10 @@ void ppce500_init(MachineState *machine)
>     memory_region_add_subregion(address_space_mem, 0, machine->ram);
>
>     dev = qdev_new("e500-ccsr");
> +    s = SYS_BUS_DEVICE(dev);
>     object_property_add_child(OBJECT(machine), "e500-ccsr", OBJECT(dev));
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    ccsr = CCSR(dev);
> -    ccsr_addr_space = &ccsr->ccsr_space;
> +    sysbus_realize_and_unref(s, &error_fatal);
> +    ccsr_addr_space = sysbus_mmio_get_region(s, 0);
>     memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
>                                 ccsr_addr_space);
>
> @@ -1270,6 +1269,7 @@ static void e500_ccsr_initfn(Object *obj)
>     PPCE500CCSRState *ccsr = CCSR(obj);
>     memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>                        MPC8544_CCSRBAR_SIZE);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
> }
>
> static const TypeInfo e500_ccsr_info = {
>

Re: [PATCH 06/23] hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal resources

[Bernhard Beschow]

Am 23. September 2024 10:28:35 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> Rather than accessing the attributes of TYPE_CCSR directly, use the SysBusDevice
>> API which exists exactly for that purpose. Furthermore, registering the memory
>> region with the SysBusDevice API makes it show up in QMP's `info qom-tree`
>> command.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/pci-host/ppce500.c | 10 +++++-----
>> hw/ppc/e500.c         |  8 ++++----
>> 2 files changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
>> index 95b983b2b3..97e5d47cec 100644
>> --- a/hw/pci-host/ppce500.c
>> +++ b/hw/pci-host/ppce500.c
>> @@ -16,7 +16,6 @@
>> 
>> #include "qemu/osdep.h"
>> #include "hw/irq.h"
>> -#include "hw/ppc/e500-ccsr.h"
>> #include "hw/qdev-properties.h"
>> #include "migration/vmstate.h"
>> #include "hw/pci/pci_device.h"
>> @@ -419,11 +418,12 @@ static const VMStateDescription vmstate_ppce500_pci = {
>> static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
>> {
>>     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
>> -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
>> -                                  "/e500-ccsr"));
>> +    SysBusDevice *ccsr = SYS_BUS_DEVICE(container_get(qdev_get_machine(),
>> +                                                      "/e500-ccsr"));
>> +    MemoryRegion *ccsr_space = sysbus_mmio_get_region(ccsr, 0);
>> 
>> -    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
>> -                             0, int128_get64(ccsr->ccsr_space.size));
>> +    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0",
>> +                             ccsr_space, 0, int128_get64(ccsr_space->size));
>
>I wonder if this really needs an alias or could the original memory region be registered as the PCI BAR region? Otherwise:

This hits an assertion when running Buildroot's qemu_ppc64_e5500_defconfig. Therefore I'll keep the alias.

Best regards,
Bernhard

>
>Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>
>Regards,
>BALATON Zoltan
>
>>     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
>> }
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index e2a4f265a5..2225533e33 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -924,7 +924,6 @@ void ppce500_init(MachineState *machine)
>>     DriveInfo *dinfo;
>>     MemoryRegion *ccsr_addr_space;
>>     SysBusDevice *s;
>> -    PPCE500CCSRState *ccsr;
>>     I2CBus *i2c;
>> 
>>     irqs = g_new0(IrqLines, smp_cpus);
>> @@ -980,10 +979,10 @@ void ppce500_init(MachineState *machine)
>>     memory_region_add_subregion(address_space_mem, 0, machine->ram);
>> 
>>     dev = qdev_new("e500-ccsr");
>> +    s = SYS_BUS_DEVICE(dev);
>>     object_property_add_child(OBJECT(machine), "e500-ccsr", OBJECT(dev));
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> -    ccsr = CCSR(dev);
>> -    ccsr_addr_space = &ccsr->ccsr_space;
>> +    sysbus_realize_and_unref(s, &error_fatal);
>> +    ccsr_addr_space = sysbus_mmio_get_region(s, 0);
>>     memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
>>                                 ccsr_addr_space);
>> 
>> @@ -1270,6 +1269,7 @@ static void e500_ccsr_initfn(Object *obj)
>>     PPCE500CCSRState *ccsr = CCSR(obj);
>>     memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>>                        MPC8544_CCSRBAR_SIZE);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>> }
>> 
>> static const TypeInfo e500_ccsr_info = {
>> 

[PATCH 07/23] hw/ppc/e500: Extract ppce500_ccsr.c

[Bernhard Beschow]

The device model already has a header file. Also extract its implementation into
an accompanying source file like other e500 devices.

This commit is also a preparation for the next commit.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 MAINTAINERS           |  2 +-
 hw/ppc/e500-ccsr.h    |  2 ++
 hw/ppc/e500.c         | 17 -----------------
 hw/ppc/ppce500_ccsr.c | 38 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/meson.build    |  1 +
 5 files changed, 42 insertions(+), 18 deletions(-)
 create mode 100644 hw/ppc/ppce500_ccsr.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ffacd60f40..b7c8b7ae72 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1433,7 +1433,7 @@ e500
 L: qemu-ppc@nongnu.org
 S: Orphan
 F: hw/ppc/e500*
-F: hw/ppc/ppce500_spin.c
+F: hw/ppc/ppce500_*.c
 F: hw/gpio/mpc8xxx.c
 F: hw/i2c/mpc_i2c.c
 F: hw/net/fsl_etsec/
diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
index 249c17be3b..3ab7e72568 100644
--- a/hw/ppc/e500-ccsr.h
+++ b/hw/ppc/e500-ccsr.h
@@ -4,6 +4,8 @@
 #include "hw/sysbus.h"
 #include "qom/object.h"
 
+#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
+
 struct PPCE500CCSRState {
     /*< private >*/
     SysBusDevice parent;
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 2225533e33..4ee4304a8a 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -61,7 +61,6 @@
 #define RAM_SIZES_ALIGN            (64 * MiB)
 
 /* TODO: parameterize */
-#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
 #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
 #define MPC8544_MSI_REGS_OFFSET   0x41600ULL
 #define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
@@ -1264,21 +1263,6 @@ void ppce500_init(MachineState *machine)
     pms->boot_info.dt_size = dt_size;
 }
 
-static void e500_ccsr_initfn(Object *obj)
-{
-    PPCE500CCSRState *ccsr = CCSR(obj);
-    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
-                       MPC8544_CCSRBAR_SIZE);
-    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
-}
-
-static const TypeInfo e500_ccsr_info = {
-    .name          = TYPE_CCSR,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(PPCE500CCSRState),
-    .instance_init = e500_ccsr_initfn,
-};
-
 static const TypeInfo ppce500_info = {
     .name          = TYPE_PPCE500_MACHINE,
     .parent        = TYPE_MACHINE,
@@ -1289,7 +1273,6 @@ static const TypeInfo ppce500_info = {
 
 static void e500_register_types(void)
 {
-    type_register_static(&e500_ccsr_info);
     type_register_static(&ppce500_info);
 }
 
diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
new file mode 100644
index 0000000000..191a9ceec3
--- /dev/null
+++ b/hw/ppc/ppce500_ccsr.c
@@ -0,0 +1,38 @@
+/*
+ * QEMU PowerPC E500 embedded processors CCSR space emulation
+ *
+ * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * Author: Yu Liu,     <yu.liu@freescale.com>
+ *
+ * This file is derived from hw/ppc440_bamboo.c,
+ * the copyright for that material belongs to the original owners.
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of  the GNU General  Public License as published by
+ * the Free Software Foundation;  either version 2 of the  License, or
+ * (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "e500-ccsr.h"
+
+static void e500_ccsr_init(Object *obj)
+{
+    PPCE500CCSRState *ccsr = CCSR(obj);
+
+    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
+                       MPC8544_CCSRBAR_SIZE);
+    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
+}
+
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_CCSR,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(PPCE500CCSRState),
+        .instance_init = e500_ccsr_init,
+    },
+};
+
+DEFINE_TYPES(types)
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 7cd9189869..43c746795a 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -81,6 +81,7 @@ ppc_ss.add(when: 'CONFIG_MPC8544DS', if_true: files('mpc8544ds.c'))
 ppc_ss.add(when: 'CONFIG_E500', if_true: files(
   'e500.c',
   'mpc8544_guts.c',
+  'ppce500_ccsr.c',
   'ppce500_spin.c'
 ))
 # PowerPC 440 Xilinx ML507 reference board.
-- 
2.46.1

Re: [PATCH 07/23] hw/ppc/e500: Extract ppce500_ccsr.c

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> The device model already has a header file. Also extract its implementation into
> an accompanying source file like other e500 devices.
>
> This commit is also a preparation for the next commit.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> MAINTAINERS           |  2 +-
> hw/ppc/e500-ccsr.h    |  2 ++
> hw/ppc/e500.c         | 17 -----------------
> hw/ppc/ppce500_ccsr.c | 38 ++++++++++++++++++++++++++++++++++++++

Maybe you could call it e500_ccsr.c and also rename the header to 
e500_ccsr.h (underscore instead of dash) to match them. Or if you want to 
match ppce500_spin.c then maybe move contents of e500-ccsr.h to e500.h?
(More below...)

> hw/ppc/meson.build    |  1 +
> 5 files changed, 42 insertions(+), 18 deletions(-)
> create mode 100644 hw/ppc/ppce500_ccsr.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ffacd60f40..b7c8b7ae72 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1433,7 +1433,7 @@ e500
> L: qemu-ppc@nongnu.org
> S: Orphan
> F: hw/ppc/e500*
> -F: hw/ppc/ppce500_spin.c
> +F: hw/ppc/ppce500_*.c
> F: hw/gpio/mpc8xxx.c
> F: hw/i2c/mpc_i2c.c
> F: hw/net/fsl_etsec/
> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
> index 249c17be3b..3ab7e72568 100644
> --- a/hw/ppc/e500-ccsr.h
> +++ b/hw/ppc/e500-ccsr.h
> @@ -4,6 +4,8 @@
> #include "hw/sysbus.h"
> #include "qom/object.h"
>
> +#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> +
> struct PPCE500CCSRState {
>     /*< private >*/
>     SysBusDevice parent;
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 2225533e33..4ee4304a8a 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -61,7 +61,6 @@
> #define RAM_SIZES_ALIGN            (64 * MiB)
>
> /* TODO: parameterize */
> -#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
> #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
> #define MPC8544_MSI_REGS_OFFSET   0x41600ULL
> #define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
> @@ -1264,21 +1263,6 @@ void ppce500_init(MachineState *machine)
>     pms->boot_info.dt_size = dt_size;
> }
>
> -static void e500_ccsr_initfn(Object *obj)
> -{
> -    PPCE500CCSRState *ccsr = CCSR(obj);
> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
> -                       MPC8544_CCSRBAR_SIZE);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
> -}
> -
> -static const TypeInfo e500_ccsr_info = {
> -    .name          = TYPE_CCSR,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(PPCE500CCSRState),
> -    .instance_init = e500_ccsr_initfn,
> -};
> -
> static const TypeInfo ppce500_info = {
>     .name          = TYPE_PPCE500_MACHINE,
>     .parent        = TYPE_MACHINE,
> @@ -1289,7 +1273,6 @@ static const TypeInfo ppce500_info = {
>
> static void e500_register_types(void)
> {
> -    type_register_static(&e500_ccsr_info);
>     type_register_static(&ppce500_info);
> }
>
> diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
> new file mode 100644
> index 0000000000..191a9ceec3
> --- /dev/null
> +++ b/hw/ppc/ppce500_ccsr.c
> @@ -0,0 +1,38 @@
> +/*
> + * QEMU PowerPC E500 embedded processors CCSR space emulation
> + *
> + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: Yu Liu,     <yu.liu@freescale.com>
> + *
> + * This file is derived from hw/ppc440_bamboo.c,
> + * the copyright for that material belongs to the original owners.

I think CCSR is a Freescale thing so likely this has nothing to do with 
ppc440_bamboo so this sentence was for other parts of e500.c not 
applicable to this part.

> + *
> + * This is free software; you can redistribute it and/or modify
> + * it under the terms of  the GNU General  Public License as published by
> + * the Free Software Foundation;  either version 2 of the  License, or
> + * (at your option) any later version.
> + */

SPDX-License-Identifier seems to be preferred by some nowadays, I don't 
have an opinion on that so just mentioning it for consideration but I'm OK 
with this one too although it seems a bit long.

Regards,
BALATON Zoltan

> +
> +#include "qemu/osdep.h"
> +#include "e500-ccsr.h"
> +
> +static void e500_ccsr_init(Object *obj)
> +{
> +    PPCE500CCSRState *ccsr = CCSR(obj);
> +
> +    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
> +                       MPC8544_CCSRBAR_SIZE);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
> +}
> +
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_CCSR,
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(PPCE500CCSRState),
> +        .instance_init = e500_ccsr_init,
> +    },
> +};
> +
> +DEFINE_TYPES(types)
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index 7cd9189869..43c746795a 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -81,6 +81,7 @@ ppc_ss.add(when: 'CONFIG_MPC8544DS', if_true: files('mpc8544ds.c'))
> ppc_ss.add(when: 'CONFIG_E500', if_true: files(
>   'e500.c',
>   'mpc8544_guts.c',
> +  'ppce500_ccsr.c',
>   'ppce500_spin.c'
> ))
> # PowerPC 440 Xilinx ML507 reference board.
>

Re: [PATCH 07/23] hw/ppc/e500: Extract ppce500_ccsr.c

[Bernhard Beschow]

Am 23. September 2024 10:38:46 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> The device model already has a header file. Also extract its implementation into
>> an accompanying source file like other e500 devices.
>> 
>> This commit is also a preparation for the next commit.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> MAINTAINERS           |  2 +-
>> hw/ppc/e500-ccsr.h    |  2 ++
>> hw/ppc/e500.c         | 17 -----------------
>> hw/ppc/ppce500_ccsr.c | 38 ++++++++++++++++++++++++++++++++++++++
>
>Maybe you could call it e500_ccsr.c and also rename the header to e500_ccsr.h (underscore instead of dash) to match them. Or if you want to match ppce500_spin.c then maybe move contents of e500-ccsr.h to e500.h?

Yeah, naming is hard. I'd actually prefer something like fsl_immr.* (taking inspiration from device tree here) to not confuse the whole SoC with the CPU. With the ppce500 prefix I was sticking to the convention of the PCIE device. Though there is also mpc-i2c and fsl_etsec...

>(More below...)
>
>> hw/ppc/meson.build    |  1 +
>> 5 files changed, 42 insertions(+), 18 deletions(-)
>> create mode 100644 hw/ppc/ppce500_ccsr.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ffacd60f40..b7c8b7ae72 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1433,7 +1433,7 @@ e500
>> L: qemu-ppc@nongnu.org
>> S: Orphan
>> F: hw/ppc/e500*
>> -F: hw/ppc/ppce500_spin.c
>> +F: hw/ppc/ppce500_*.c
>> F: hw/gpio/mpc8xxx.c
>> F: hw/i2c/mpc_i2c.c
>> F: hw/net/fsl_etsec/
>> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
>> index 249c17be3b..3ab7e72568 100644
>> --- a/hw/ppc/e500-ccsr.h
>> +++ b/hw/ppc/e500-ccsr.h
>> @@ -4,6 +4,8 @@
>> #include "hw/sysbus.h"
>> #include "qom/object.h"
>> 
>> +#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> +
>> struct PPCE500CCSRState {
>>     /*< private >*/
>>     SysBusDevice parent;
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 2225533e33..4ee4304a8a 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -61,7 +61,6 @@
>> #define RAM_SIZES_ALIGN            (64 * MiB)
>> 
>> /* TODO: parameterize */
>> -#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>> #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
>> #define MPC8544_MSI_REGS_OFFSET   0x41600ULL
>> #define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
>> @@ -1264,21 +1263,6 @@ void ppce500_init(MachineState *machine)
>>     pms->boot_info.dt_size = dt_size;
>> }
>> 
>> -static void e500_ccsr_initfn(Object *obj)
>> -{
>> -    PPCE500CCSRState *ccsr = CCSR(obj);
>> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>> -                       MPC8544_CCSRBAR_SIZE);
>> -    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>> -}
>> -
>> -static const TypeInfo e500_ccsr_info = {
>> -    .name          = TYPE_CCSR,
>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>> -    .instance_size = sizeof(PPCE500CCSRState),
>> -    .instance_init = e500_ccsr_initfn,
>> -};
>> -
>> static const TypeInfo ppce500_info = {
>>     .name          = TYPE_PPCE500_MACHINE,
>>     .parent        = TYPE_MACHINE,
>> @@ -1289,7 +1273,6 @@ static const TypeInfo ppce500_info = {
>> 
>> static void e500_register_types(void)
>> {
>> -    type_register_static(&e500_ccsr_info);
>>     type_register_static(&ppce500_info);
>> }
>> 
>> diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
>> new file mode 100644
>> index 0000000000..191a9ceec3
>> --- /dev/null
>> +++ b/hw/ppc/ppce500_ccsr.c
>> @@ -0,0 +1,38 @@
>> +/*
>> + * QEMU PowerPC E500 embedded processors CCSR space emulation
>> + *
>> + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
>> + *
>> + * Author: Yu Liu,     <yu.liu@freescale.com>
>> + *
>> + * This file is derived from hw/ppc440_bamboo.c,
>> + * the copyright for that material belongs to the original owners.
>
>I think CCSR is a Freescale thing so likely this has nothing to do with ppc440_bamboo so this sentence was for other parts of e500.c not applicable to this part.

Good point. IANAL so erred on the safe side and copied the whole license text. I'm happy to omit this and would also prefer SPDX license identifiers as long as I get the confirmation that this is legal.

Best regards,
Bernhard

>
>> + *
>> + * This is free software; you can redistribute it and/or modify
>> + * it under the terms of  the GNU General  Public License as published by
>> + * the Free Software Foundation;  either version 2 of the  License, or
>> + * (at your option) any later version.
>> + */
>
>SPDX-License-Identifier seems to be preferred by some nowadays, I don't have an opinion on that so just mentioning it for consideration but I'm OK with this one too although it seems a bit long.
>
>Regards,
>BALATON Zoltan
>
>> +
>> +#include "qemu/osdep.h"
>> +#include "e500-ccsr.h"
>> +
>> +static void e500_ccsr_init(Object *obj)
>> +{
>> +    PPCE500CCSRState *ccsr = CCSR(obj);
>> +
>> +    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>> +                       MPC8544_CCSRBAR_SIZE);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>> +}
>> +
>> +static const TypeInfo types[] = {
>> +    {
>> +        .name          = TYPE_CCSR,
>> +        .parent        = TYPE_SYS_BUS_DEVICE,
>> +        .instance_size = sizeof(PPCE500CCSRState),
>> +        .instance_init = e500_ccsr_init,
>> +    },
>> +};
>> +
>> +DEFINE_TYPES(types)
>> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
>> index 7cd9189869..43c746795a 100644
>> --- a/hw/ppc/meson.build
>> +++ b/hw/ppc/meson.build
>> @@ -81,6 +81,7 @@ ppc_ss.add(when: 'CONFIG_MPC8544DS', if_true: files('mpc8544ds.c'))
>> ppc_ss.add(when: 'CONFIG_E500', if_true: files(
>>   'e500.c',
>>   'mpc8544_guts.c',
>> +  'ppce500_ccsr.c',
>>   'ppce500_spin.c'
>> ))
>> # PowerPC 440 Xilinx ML507 reference board.
>> 

Re: [PATCH 07/23] hw/ppc/e500: Extract ppce500_ccsr.c

[Bernhard Beschow]

Am 24. September 2024 20:02:31 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 23. September 2024 10:38:46 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>>> The device model already has a header file. Also extract its implementation into
>>> an accompanying source file like other e500 devices.
>>> 
>>> This commit is also a preparation for the next commit.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> MAINTAINERS           |  2 +-
>>> hw/ppc/e500-ccsr.h    |  2 ++
>>> hw/ppc/e500.c         | 17 -----------------
>>> hw/ppc/ppce500_ccsr.c | 38 ++++++++++++++++++++++++++++++++++++++
>>
>>Maybe you could call it e500_ccsr.c and also rename the header to e500_ccsr.h (underscore instead of dash) to match them. Or if you want to match ppce500_spin.c then maybe move contents of e500-ccsr.h to e500.h?
>
>Yeah, naming is hard. I'd actually prefer something like fsl_immr.* (taking inspiration from device tree here) to not confuse the whole SoC with the CPU. With the ppce500 prefix I was sticking to the convention of the PCIE device. Though there is also mpc-i2c and fsl_etsec...

I'll name the header like the struct it defines, i.e. ppce500_ccsr.h.

>
>>(More below...)
>>
>>> hw/ppc/meson.build    |  1 +
>>> 5 files changed, 42 insertions(+), 18 deletions(-)
>>> create mode 100644 hw/ppc/ppce500_ccsr.c
>>> 
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index ffacd60f40..b7c8b7ae72 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1433,7 +1433,7 @@ e500
>>> L: qemu-ppc@nongnu.org
>>> S: Orphan
>>> F: hw/ppc/e500*
>>> -F: hw/ppc/ppce500_spin.c
>>> +F: hw/ppc/ppce500_*.c
>>> F: hw/gpio/mpc8xxx.c
>>> F: hw/i2c/mpc_i2c.c
>>> F: hw/net/fsl_etsec/
>>> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
>>> index 249c17be3b..3ab7e72568 100644
>>> --- a/hw/ppc/e500-ccsr.h
>>> +++ b/hw/ppc/e500-ccsr.h
>>> @@ -4,6 +4,8 @@
>>> #include "hw/sysbus.h"
>>> #include "qom/object.h"
>>> 
>>> +#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>>> +
>>> struct PPCE500CCSRState {
>>>     /*< private >*/
>>>     SysBusDevice parent;
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index 2225533e33..4ee4304a8a 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -61,7 +61,6 @@
>>> #define RAM_SIZES_ALIGN            (64 * MiB)
>>> 
>>> /* TODO: parameterize */
>>> -#define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>>> #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
>>> #define MPC8544_MSI_REGS_OFFSET   0x41600ULL
>>> #define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
>>> @@ -1264,21 +1263,6 @@ void ppce500_init(MachineState *machine)
>>>     pms->boot_info.dt_size = dt_size;
>>> }
>>> 
>>> -static void e500_ccsr_initfn(Object *obj)
>>> -{
>>> -    PPCE500CCSRState *ccsr = CCSR(obj);
>>> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>>> -                       MPC8544_CCSRBAR_SIZE);
>>> -    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>>> -}
>>> -
>>> -static const TypeInfo e500_ccsr_info = {
>>> -    .name          = TYPE_CCSR,
>>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>>> -    .instance_size = sizeof(PPCE500CCSRState),
>>> -    .instance_init = e500_ccsr_initfn,
>>> -};
>>> -
>>> static const TypeInfo ppce500_info = {
>>>     .name          = TYPE_PPCE500_MACHINE,
>>>     .parent        = TYPE_MACHINE,
>>> @@ -1289,7 +1273,6 @@ static const TypeInfo ppce500_info = {
>>> 
>>> static void e500_register_types(void)
>>> {
>>> -    type_register_static(&e500_ccsr_info);
>>>     type_register_static(&ppce500_info);
>>> }
>>> 
>>> diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
>>> new file mode 100644
>>> index 0000000000..191a9ceec3
>>> --- /dev/null
>>> +++ b/hw/ppc/ppce500_ccsr.c
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * QEMU PowerPC E500 embedded processors CCSR space emulation
>>> + *
>>> + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
>>> + *
>>> + * Author: Yu Liu,     <yu.liu@freescale.com>
>>> + *
>>> + * This file is derived from hw/ppc440_bamboo.c,
>>> + * the copyright for that material belongs to the original owners.
>>
>>I think CCSR is a Freescale thing so likely this has nothing to do with ppc440_bamboo so this sentence was for other parts of e500.c not applicable to this part.
>
>Good point. IANAL so erred on the safe side and copied the whole license text. I'm happy to omit this and would also prefer SPDX license identifiers as long as I get the confirmation that this is legal.

I'll remove the reference to ppc440_bamboo.c which is clearly not relevant here. Beyond that I rather leave the license text unchanged.

Best regards,
Bernhard

>
>Best regards,
>Bernhard
>
>>
>>> + *
>>> + * This is free software; you can redistribute it and/or modify
>>> + * it under the terms of  the GNU General  Public License as published by
>>> + * the Free Software Foundation;  either version 2 of the  License, or
>>> + * (at your option) any later version.
>>> + */
>>
>>SPDX-License-Identifier seems to be preferred by some nowadays, I don't have an opinion on that so just mentioning it for consideration but I'm OK with this one too although it seems a bit long.
>>
>>Regards,
>>BALATON Zoltan
>>
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "e500-ccsr.h"
>>> +
>>> +static void e500_ccsr_init(Object *obj)
>>> +{
>>> +    PPCE500CCSRState *ccsr = CCSR(obj);
>>> +
>>> +    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>>> +                       MPC8544_CCSRBAR_SIZE);
>>> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>>> +}
>>> +
>>> +static const TypeInfo types[] = {
>>> +    {
>>> +        .name          = TYPE_CCSR,
>>> +        .parent        = TYPE_SYS_BUS_DEVICE,
>>> +        .instance_size = sizeof(PPCE500CCSRState),
>>> +        .instance_init = e500_ccsr_init,
>>> +    },
>>> +};
>>> +
>>> +DEFINE_TYPES(types)
>>> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
>>> index 7cd9189869..43c746795a 100644
>>> --- a/hw/ppc/meson.build
>>> +++ b/hw/ppc/meson.build
>>> @@ -81,6 +81,7 @@ ppc_ss.add(when: 'CONFIG_MPC8544DS', if_true: files('mpc8544ds.c'))
>>> ppc_ss.add(when: 'CONFIG_E500', if_true: files(
>>>   'e500.c',
>>>   'mpc8544_guts.c',
>>> +  'ppce500_ccsr.c',
>>>   'ppce500_spin.c'
>>> ))
>>> # PowerPC 440 Xilinx ML507 reference board.
>>> 

[PATCH 08/23] hw/ppc/ppce500_ccsr: Log access to unimplemented registers

[Bernhard Beschow]

The CCSR space is just a container which is meant to be covered by platform
device memory regions. However, QEMU only implements a subset of these devices.
Add some logging to see which devices a guest attempts to access.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/ppce500_ccsr.c | 33 +++++++++++++++++++++++++++++++--
 hw/ppc/trace-events   |  3 +++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
index 191a9ceec3..28942b2348 100644
--- a/hw/ppc/ppce500_ccsr.c
+++ b/hw/ppc/ppce500_ccsr.c
@@ -15,14 +15,43 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "e500-ccsr.h"
+#include "trace.h"
+
+static uint64_t ppce500_ccsr_io_read(void *opaque, hwaddr addr, unsigned size)
+{
+    uint64_t value = 0;
+
+    trace_ppce500_ccsr_io_read(addr, value, size);
+    qemu_log_mask(LOG_UNIMP,
+                  "%s: unimplemented [0x%" HWADDR_PRIx "] -> 0\n",
+                  __func__, addr);
+
+    return value;
+}
+
+static void ppce500_ccsr_io_write(void *opaque, hwaddr addr, uint64_t value,
+                                  unsigned size)
+{
+    trace_ppce500_ccsr_io_write(addr, value, size);
+    qemu_log_mask(LOG_UNIMP,
+                  "%s: unimplemented [0x%" HWADDR_PRIx "] <- 0x%" PRIx32 "\n",
+                  __func__, addr, (uint32_t)value);
+}
+
+static const MemoryRegionOps ppce500_ccsr_ops = {
+    .read = ppce500_ccsr_io_read,
+    .write = ppce500_ccsr_io_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
 
 static void e500_ccsr_init(Object *obj)
 {
     PPCE500CCSRState *ccsr = CCSR(obj);
 
-    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
-                       MPC8544_CCSRBAR_SIZE);
+    memory_region_init_io(&ccsr->ccsr_space, obj, &ppce500_ccsr_ops, obj,
+                          "e500-ccsr", MPC8544_CCSRBAR_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
 }
 
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 1f125ce841..ca4c231c9f 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -143,6 +143,9 @@ ppc_irq_cpu(const char *action) "%s"
 ppc_dcr_read(uint32_t addr, uint32_t val) "DRCN[0x%x] -> 0x%x"
 ppc_dcr_write(uint32_t addr, uint32_t val) "DRCN[0x%x] <- 0x%x"
 
+ppce500_ccsr_io_read(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] -> 0x%08x (size: 0x%" PRIu8 ")"
+ppce500_ccsr_io_write(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] <- 0x%08x (size: 0x%" PRIu8 ")"
+
 # prep_systemio.c
 prep_systemio_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
 prep_systemio_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
-- 
2.46.1

Re: [PATCH 08/23] hw/ppc/ppce500_ccsr: Log access to unimplemented registers

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> The CCSR space is just a container which is meant to be covered by platform
> device memory regions. However, QEMU only implements a subset of these devices.
> Add some logging to see which devices a guest attempts to access.

An aleternative solution for a similar problem is this:
https://patchew.org/QEMU/20240520101007.A25A34E602E@zero.eik.bme.hu/
I don't know if that would be simpler for this device as well.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/ppc/ppce500_ccsr.c | 33 +++++++++++++++++++++++++++++++--
> hw/ppc/trace-events   |  3 +++
> 2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
> index 191a9ceec3..28942b2348 100644
> --- a/hw/ppc/ppce500_ccsr.c
> +++ b/hw/ppc/ppce500_ccsr.c
> @@ -15,14 +15,43 @@
>  */
>
> #include "qemu/osdep.h"
> +#include "qemu/log.h"
> #include "e500-ccsr.h"
> +#include "trace.h"
> +
> +static uint64_t ppce500_ccsr_io_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    uint64_t value = 0;
> +
> +    trace_ppce500_ccsr_io_read(addr, value, size);
> +    qemu_log_mask(LOG_UNIMP,
> +                  "%s: unimplemented [0x%" HWADDR_PRIx "] -> 0\n",
> +                  __func__, addr);
> +
> +    return value;
> +}
> +
> +static void ppce500_ccsr_io_write(void *opaque, hwaddr addr, uint64_t value,
> +                                  unsigned size)
> +{
> +    trace_ppce500_ccsr_io_write(addr, value, size);
> +    qemu_log_mask(LOG_UNIMP,
> +                  "%s: unimplemented [0x%" HWADDR_PRIx "] <- 0x%" PRIx32 "\n",
> +                  __func__, addr, (uint32_t)value);
> +}
> +
> +static const MemoryRegionOps ppce500_ccsr_ops = {
> +    .read = ppce500_ccsr_io_read,
> +    .write = ppce500_ccsr_io_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
>
> static void e500_ccsr_init(Object *obj)
> {
>     PPCE500CCSRState *ccsr = CCSR(obj);
>
> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
> -                       MPC8544_CCSRBAR_SIZE);
> +    memory_region_init_io(&ccsr->ccsr_space, obj, &ppce500_ccsr_ops, obj,
> +                          "e500-ccsr", MPC8544_CCSRBAR_SIZE);
>     sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
> }
>
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 1f125ce841..ca4c231c9f 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -143,6 +143,9 @@ ppc_irq_cpu(const char *action) "%s"
> ppc_dcr_read(uint32_t addr, uint32_t val) "DRCN[0x%x] -> 0x%x"
> ppc_dcr_write(uint32_t addr, uint32_t val) "DRCN[0x%x] <- 0x%x"
>
> +ppce500_ccsr_io_read(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] -> 0x%08x (size: 0x%" PRIu8 ")"
> +ppce500_ccsr_io_write(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] <- 0x%08x (size: 0x%" PRIu8 ")"
> +
> # prep_systemio.c
> prep_systemio_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
> prep_systemio_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
>

Re: [PATCH 08/23] hw/ppc/ppce500_ccsr: Log access to unimplemented registers

[Bernhard Beschow]

Am 24. September 2024 10:15:43 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> The CCSR space is just a container which is meant to be covered by platform
>> device memory regions. However, QEMU only implements a subset of these devices.
>> Add some logging to see which devices a guest attempts to access.
>
>An aleternative solution for a similar problem is this:
>https://patchew.org/QEMU/20240520101007.A25A34E602E@zero.eik.bme.hu/
>I don't know if that would be simpler for this device as well.

That was my first approach but I didn't like that `-d  unimp` causes unrelated logging. With tracing one can be very targeted.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/ppce500_ccsr.c | 33 +++++++++++++++++++++++++++++++--
>> hw/ppc/trace-events   |  3 +++
>> 2 files changed, 34 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/ppce500_ccsr.c b/hw/ppc/ppce500_ccsr.c
>> index 191a9ceec3..28942b2348 100644
>> --- a/hw/ppc/ppce500_ccsr.c
>> +++ b/hw/ppc/ppce500_ccsr.c
>> @@ -15,14 +15,43 @@
>>  */
>> 
>> #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> #include "e500-ccsr.h"
>> +#include "trace.h"
>> +
>> +static uint64_t ppce500_ccsr_io_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    uint64_t value = 0;
>> +
>> +    trace_ppce500_ccsr_io_read(addr, value, size);
>> +    qemu_log_mask(LOG_UNIMP,
>> +                  "%s: unimplemented [0x%" HWADDR_PRIx "] -> 0\n",
>> +                  __func__, addr);
>> +
>> +    return value;
>> +}
>> +
>> +static void ppce500_ccsr_io_write(void *opaque, hwaddr addr, uint64_t value,
>> +                                  unsigned size)
>> +{
>> +    trace_ppce500_ccsr_io_write(addr, value, size);
>> +    qemu_log_mask(LOG_UNIMP,
>> +                  "%s: unimplemented [0x%" HWADDR_PRIx "] <- 0x%" PRIx32 "\n",
>> +                  __func__, addr, (uint32_t)value);
>> +}
>> +
>> +static const MemoryRegionOps ppce500_ccsr_ops = {
>> +    .read = ppce500_ccsr_io_read,
>> +    .write = ppce500_ccsr_io_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> 
>> static void e500_ccsr_init(Object *obj)
>> {
>>     PPCE500CCSRState *ccsr = CCSR(obj);
>> 
>> -    memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>> -                       MPC8544_CCSRBAR_SIZE);
>> +    memory_region_init_io(&ccsr->ccsr_space, obj, &ppce500_ccsr_ops, obj,
>> +                          "e500-ccsr", MPC8544_CCSRBAR_SIZE);
>>     sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>> }
>> 
>> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
>> index 1f125ce841..ca4c231c9f 100644
>> --- a/hw/ppc/trace-events
>> +++ b/hw/ppc/trace-events
>> @@ -143,6 +143,9 @@ ppc_irq_cpu(const char *action) "%s"
>> ppc_dcr_read(uint32_t addr, uint32_t val) "DRCN[0x%x] -> 0x%x"
>> ppc_dcr_write(uint32_t addr, uint32_t val) "DRCN[0x%x] <- 0x%x"
>> 
>> +ppce500_ccsr_io_read(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] -> 0x%08x (size: 0x%" PRIu8 ")"
>> +ppce500_ccsr_io_write(uint32_t index, uint32_t val, uint8_t size) "[0x%" PRIx32 "] <- 0x%08x (size: 0x%" PRIu8 ")"
>> +
>> # prep_systemio.c
>> prep_systemio_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
>> prep_systemio_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
>> 

[PATCH 09/23] hw/ppc/mpc8544_guts: Populate POR PLL ratio status register

[Bernhard Beschow]

Populate this read-only register with some arbitrary values which avoids
U-Boot's get_clocks() to hang().

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

diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
index e3540b0281..6688fd44c3 100644
--- a/hw/ppc/mpc8544_guts.c
+++ b/hw/ppc/mpc8544_guts.c
@@ -29,6 +29,12 @@
 #define MPC8544_GUTS_RSTCR_RESET      0x02
 
 #define MPC8544_GUTS_ADDR_PORPLLSR    0x00
+REG32(GUTS_PORPLLSR, 0x00)
+    FIELD(GUTS_PORPLLSR, E500_1_RATIO, 16, 6)
+    FIELD(GUTS_PORPLLSR, E500_0_RATIO, 16, 6)
+    FIELD(GUTS_PORPLLSR, DDR_RATIO, 9, 5)
+    FIELD(GUTS_PORPLLSR, PLAT_RATIO, 1, 5)
+
 #define MPC8544_GUTS_ADDR_PORBMSR     0x04
 #define MPC8544_GUTS_ADDR_PORIMPSCR   0x08
 #define MPC8544_GUTS_ADDR_PORDEVSR    0x0C
@@ -75,6 +81,12 @@ static uint64_t mpc8544_guts_read(void *opaque, hwaddr addr,
 
     addr &= MPC8544_GUTS_MMIO_SIZE - 1;
     switch (addr) {
+    case MPC8544_GUTS_ADDR_PORPLLSR:
+        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_1_RATIO, 3); /* 3:2 */
+        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_0_RATIO, 3); /* 3:2 */
+        value = FIELD_DP32(value, GUTS_PORPLLSR, DDR_RATIO, 6); /* 6:1 */
+        value = FIELD_DP32(value, GUTS_PORPLLSR, PLAT_RATIO, 4); /* 4:1 */
+        break;
     case MPC8544_GUTS_ADDR_PVR:
         value = env->spr[SPR_PVR];
         break;
-- 
2.46.1

Re: [PATCH 09/23] hw/ppc/mpc8544_guts: Populate POR PLL ratio status register

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Populate this read-only register with some arbitrary values which avoids
> U-Boot's get_clocks() to hang().

Maybe this should be a property settable by the machine as each board may 
have different values and it may need to use the correct value for the 
machine.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/ppc/mpc8544_guts.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
> index e3540b0281..6688fd44c3 100644
> --- a/hw/ppc/mpc8544_guts.c
> +++ b/hw/ppc/mpc8544_guts.c
> @@ -29,6 +29,12 @@
> #define MPC8544_GUTS_RSTCR_RESET      0x02
>
> #define MPC8544_GUTS_ADDR_PORPLLSR    0x00
> +REG32(GUTS_PORPLLSR, 0x00)
> +    FIELD(GUTS_PORPLLSR, E500_1_RATIO, 16, 6)
> +    FIELD(GUTS_PORPLLSR, E500_0_RATIO, 16, 6)
> +    FIELD(GUTS_PORPLLSR, DDR_RATIO, 9, 5)
> +    FIELD(GUTS_PORPLLSR, PLAT_RATIO, 1, 5)
> +
> #define MPC8544_GUTS_ADDR_PORBMSR     0x04
> #define MPC8544_GUTS_ADDR_PORIMPSCR   0x08
> #define MPC8544_GUTS_ADDR_PORDEVSR    0x0C
> @@ -75,6 +81,12 @@ static uint64_t mpc8544_guts_read(void *opaque, hwaddr addr,
>
>     addr &= MPC8544_GUTS_MMIO_SIZE - 1;
>     switch (addr) {
> +    case MPC8544_GUTS_ADDR_PORPLLSR:
> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_1_RATIO, 3); /* 3:2 */
> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_0_RATIO, 3); /* 3:2 */
> +        value = FIELD_DP32(value, GUTS_PORPLLSR, DDR_RATIO, 6); /* 6:1 */
> +        value = FIELD_DP32(value, GUTS_PORPLLSR, PLAT_RATIO, 4); /* 4:1 */
> +        break;
>     case MPC8544_GUTS_ADDR_PVR:
>         value = env->spr[SPR_PVR];
>         break;
>

Re: [PATCH 09/23] hw/ppc/mpc8544_guts: Populate POR PLL ratio status register

[Bernhard Beschow]

Am 23. September 2024 10:43:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> Populate this read-only register with some arbitrary values which avoids
>> U-Boot's get_clocks() to hang().
>
>Maybe this should be a property settable by the machine as each board may have different values and it may need to use the correct value for the machine.

I actually considered this but went with the pragmatic solution to avoid over-engineering. In particular, I wanted to avoid further machine-specitic attributes in the machine class struct. Or do you expect a new e500 machine to be added? In that case I'd set above arbitrary values as default and expect a new machine to override these properties.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/mpc8544_guts.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>> 
>> diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
>> index e3540b0281..6688fd44c3 100644
>> --- a/hw/ppc/mpc8544_guts.c
>> +++ b/hw/ppc/mpc8544_guts.c
>> @@ -29,6 +29,12 @@
>> #define MPC8544_GUTS_RSTCR_RESET      0x02
>> 
>> #define MPC8544_GUTS_ADDR_PORPLLSR    0x00
>> +REG32(GUTS_PORPLLSR, 0x00)
>> +    FIELD(GUTS_PORPLLSR, E500_1_RATIO, 16, 6)
>> +    FIELD(GUTS_PORPLLSR, E500_0_RATIO, 16, 6)
>> +    FIELD(GUTS_PORPLLSR, DDR_RATIO, 9, 5)
>> +    FIELD(GUTS_PORPLLSR, PLAT_RATIO, 1, 5)
>> +
>> #define MPC8544_GUTS_ADDR_PORBMSR     0x04
>> #define MPC8544_GUTS_ADDR_PORIMPSCR   0x08
>> #define MPC8544_GUTS_ADDR_PORDEVSR    0x0C
>> @@ -75,6 +81,12 @@ static uint64_t mpc8544_guts_read(void *opaque, hwaddr addr,
>> 
>>     addr &= MPC8544_GUTS_MMIO_SIZE - 1;
>>     switch (addr) {
>> +    case MPC8544_GUTS_ADDR_PORPLLSR:
>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_1_RATIO, 3); /* 3:2 */
>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_0_RATIO, 3); /* 3:2 */
>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, DDR_RATIO, 6); /* 6:1 */
>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, PLAT_RATIO, 4); /* 4:1 */
>> +        break;
>>     case MPC8544_GUTS_ADDR_PVR:
>>         value = env->spr[SPR_PVR];
>>         break;
>> 

Re: [PATCH 09/23] hw/ppc/mpc8544_guts: Populate POR PLL ratio status register

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Am 23. September 2024 10:43:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>>> Populate this read-only register with some arbitrary values which avoids
>>> U-Boot's get_clocks() to hang().
>>
>> Maybe this should be a property settable by the machine as each board 
>> may have different values and it may need to use the correct value for 
>> the machine.
>
> I actually considered this but went with the pragmatic solution to avoid 
> over-engineering. In particular, I wanted to avoid further 
> machine-specitic attributes in the machine class struct. Or do you 
> expect a new e500 machine to be added? In that case I'd set above 
> arbitrary values as default and expect a new machine to override these 
> properties.

Can't override if there's no property for it. There's one machine I may be 
interested in that uses a Freescale e500 SoC. That one seems to use 
0x0606180c for this value which I think corresponds to 0/1 Ratio both 3:1, 
DDR Ratio 12:1 and Plat Ratio 6:1. I think one property to set the 32 bit 
value without individual fields would be enough and we can put comments 
next to the value if needed to note what components it comes from. Or if 
you just need any value here maybe you could take this one then that would 
be good for me as well. (I have some patches adding second i2c bus and SPD 
data that are needed for U-Boot for memory detection but it needs more 
clean up before I can submit it and also waiting for these patches to 
avoid conflict.)

Regards,
BALATON Zoltan

>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/ppc/mpc8544_guts.c | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
>>> index e3540b0281..6688fd44c3 100644
>>> --- a/hw/ppc/mpc8544_guts.c
>>> +++ b/hw/ppc/mpc8544_guts.c
>>> @@ -29,6 +29,12 @@
>>> #define MPC8544_GUTS_RSTCR_RESET      0x02
>>>
>>> #define MPC8544_GUTS_ADDR_PORPLLSR    0x00
>>> +REG32(GUTS_PORPLLSR, 0x00)
>>> +    FIELD(GUTS_PORPLLSR, E500_1_RATIO, 16, 6)
>>> +    FIELD(GUTS_PORPLLSR, E500_0_RATIO, 16, 6)
>>> +    FIELD(GUTS_PORPLLSR, DDR_RATIO, 9, 5)
>>> +    FIELD(GUTS_PORPLLSR, PLAT_RATIO, 1, 5)
>>> +
>>> #define MPC8544_GUTS_ADDR_PORBMSR     0x04
>>> #define MPC8544_GUTS_ADDR_PORIMPSCR   0x08
>>> #define MPC8544_GUTS_ADDR_PORDEVSR    0x0C
>>> @@ -75,6 +81,12 @@ static uint64_t mpc8544_guts_read(void *opaque, hwaddr addr,
>>>
>>>     addr &= MPC8544_GUTS_MMIO_SIZE - 1;
>>>     switch (addr) {
>>> +    case MPC8544_GUTS_ADDR_PORPLLSR:
>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_1_RATIO, 3); /* 3:2 */
>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_0_RATIO, 3); /* 3:2 */
>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, DDR_RATIO, 6); /* 6:1 */
>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, PLAT_RATIO, 4); /* 4:1 */
>>> +        break;
>>>     case MPC8544_GUTS_ADDR_PVR:
>>>         value = env->spr[SPR_PVR];
>>>         break;
>>>
>
>

Re: [PATCH 09/23] hw/ppc/mpc8544_guts: Populate POR PLL ratio status register

[Bernhard Beschow]

Am 24. September 2024 09:59:21 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> Am 23. September 2024 10:43:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>>>> Populate this read-only register with some arbitrary values which avoids
>>>> U-Boot's get_clocks() to hang().
>>> 
>>> Maybe this should be a property settable by the machine as each board may have different values and it may need to use the correct value for the machine.
>> 
>> I actually considered this but went with the pragmatic solution to avoid over-engineering. In particular, I wanted to avoid further machine-specitic attributes in the machine class struct. Or do you expect a new e500 machine to be added? In that case I'd set above arbitrary values as default and expect a new machine to override these properties.
>
>Can't override if there's no property for it. There's one machine I may be interested in that uses a Freescale e500 SoC. That one seems to use 0x0606180c for this value which I think corresponds to 0/1 Ratio both 3:1, DDR Ratio 12:1 and Plat Ratio 6:1. I think one property to set the 32 bit value without individual fields would be enough and we can put comments next to the value if needed to note what components it comes from. Or if you just need any value here maybe you could take this one then that would be good for me as well.

Let's use your numbers then without a property. Any specific machine I should mention in the commit message? Tabor?

> (I have some patches adding second i2c bus and SPD data that are needed for U-Boot for memory detection but it needs more clean up before I can submit it and also waiting for these patches to avoid conflict.)

Neat. That means we'll get DDR3 support?

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> hw/ppc/mpc8544_guts.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>> 
>>>> diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
>>>> index e3540b0281..6688fd44c3 100644
>>>> --- a/hw/ppc/mpc8544_guts.c
>>>> +++ b/hw/ppc/mpc8544_guts.c
>>>> @@ -29,6 +29,12 @@
>>>> #define MPC8544_GUTS_RSTCR_RESET      0x02
>>>> 
>>>> #define MPC8544_GUTS_ADDR_PORPLLSR    0x00
>>>> +REG32(GUTS_PORPLLSR, 0x00)
>>>> +    FIELD(GUTS_PORPLLSR, E500_1_RATIO, 16, 6)
>>>> +    FIELD(GUTS_PORPLLSR, E500_0_RATIO, 16, 6)
>>>> +    FIELD(GUTS_PORPLLSR, DDR_RATIO, 9, 5)
>>>> +    FIELD(GUTS_PORPLLSR, PLAT_RATIO, 1, 5)
>>>> +
>>>> #define MPC8544_GUTS_ADDR_PORBMSR     0x04
>>>> #define MPC8544_GUTS_ADDR_PORIMPSCR   0x08
>>>> #define MPC8544_GUTS_ADDR_PORDEVSR    0x0C
>>>> @@ -75,6 +81,12 @@ static uint64_t mpc8544_guts_read(void *opaque, hwaddr addr,
>>>> 
>>>>     addr &= MPC8544_GUTS_MMIO_SIZE - 1;
>>>>     switch (addr) {
>>>> +    case MPC8544_GUTS_ADDR_PORPLLSR:
>>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_1_RATIO, 3); /* 3:2 */
>>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, E500_0_RATIO, 3); /* 3:2 */
>>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, DDR_RATIO, 6); /* 6:1 */
>>>> +        value = FIELD_DP32(value, GUTS_PORPLLSR, PLAT_RATIO, 4); /* 4:1 */
>>>> +        break;
>>>>     case MPC8544_GUTS_ADDR_PVR:
>>>>         value = env->spr[SPR_PVR];
>>>>         break;
>>>> 
>> 
>> 

Re: [PATCH 09/23] hw/ppc/mpc8544_guts: Populate POR PLL ratio status register

[BALATON Zoltan]

On Tue, 24 Sep 2024, Bernhard Beschow wrote:
> Am 24. September 2024 09:59:21 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>>> Am 23. September 2024 10:43:19 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>>>>> Populate this read-only register with some arbitrary values which avoids
>>>>> U-Boot's get_clocks() to hang().
>>>>
>>>> Maybe this should be a property settable by the machine as each board may have different values and it may need to use the correct value for the machine.
>>>
>>> I actually considered this but went with the pragmatic solution to avoid over-engineering. In particular, I wanted to avoid further machine-specitic attributes in the machine class struct. Or do you expect a new e500 machine to be added? In that case I'd set above arbitrary values as default and expect a new machine to override these properties.
>>
>> Can't override if there's no property for it. There's one machine I may be interested in that uses a Freescale e500 SoC. That one seems to use 0x0606180c for this value which I think corresponds to 0/1 Ratio both 3:1, DDR Ratio 12:1 and Plat Ratio 6:1. I think one property to set the 32 bit value without individual fields would be enough and we can put comments next to the value if needed to note what components it comes from. Or if you just need any value here maybe you could take this one then that would be good for me as well.
>
> Let's use your numbers then without a property. Any specific machine I 
> should mention in the commit message? Tabor?

That's also OK with me. No need to mention any specific machine as I'm not 
sure I'll ever finish it. So far it's only an experiment and only planned 
to submit the small patches that came out of it but it's not emulating any 
machine yet.

>> (I have some patches adding second i2c bus and SPD data that are needed for U-Boot for memory detection but it needs more clean up before I can submit it and also waiting for these patches to avoid conflict.)
>
> Neat. That means we'll get DDR3 support?

That's the plan eventually but I don't know when will I have time to do 
that. So far I had a hard coded SPD data for testing but I wanted to 
update the generating function for a while but lacked time for it. This 
gives another reason to do that finally but may take a while.

Regards,
BALATON Zoltan

[PATCH 10/23] hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i2c/mpc_i2c.c    | 9 +++++----
 hw/i2c/trace-events | 5 +++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
index 2467d1a9aa..3d79c15653 100644
--- a/hw/i2c/mpc_i2c.c
+++ b/hw/i2c/mpc_i2c.c
@@ -24,6 +24,7 @@
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "qom/object.h"
+#include "trace.h"
 
 /* #define DEBUG_I2C */
 
@@ -224,8 +225,8 @@ static uint64_t mpc_i2c_read(void *opaque, hwaddr addr, unsigned size)
         break;
     }
 
-    DPRINTF("%s: addr " HWADDR_FMT_plx " %02" PRIx32 "\n", __func__,
-                                         addr, value);
+    trace_mpc_i2c_read(addr, value);
+
     return (uint64_t)value;
 }
 
@@ -234,8 +235,8 @@ static void mpc_i2c_write(void *opaque, hwaddr addr,
 {
     MPCI2CState *s = opaque;
 
-    DPRINTF("%s: addr " HWADDR_FMT_plx " val %08" PRIx64 "\n", __func__,
-                                             addr, value);
+    trace_mpc_i2c_write(addr, value);
+
     switch (addr) {
     case MPC_I2C_ADR:
         s->adr = value & CADR_MASK;
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 6900e06eda..f708a7ace1 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -35,6 +35,11 @@ aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t va
 aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
 aspeed_i2c_bus_recv(const char *mode, int i, int count, uint8_t byte) "%s recv %d/%d 0x%02x"
 
+# mpc_i2c.c
+
+mpc_i2c_read(uint64_t addr, uint32_t value) "[0x%" PRIx64 "] -> 0x%02" PRIx32
+mpc_i2c_write(uint64_t addr, uint32_t value) "[0x%" PRIx64 "] <- 0x%02" PRIx32
+
 # npcm7xx_smbus.c
 
 npcm7xx_smbus_read(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
-- 
2.46.1

Re: [PATCH 10/23] hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   hw/i2c/mpc_i2c.c    | 9 +++++----
>   hw/i2c/trace-events | 5 +++++
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
> index 2467d1a9aa..3d79c15653 100644
> --- a/hw/i2c/mpc_i2c.c
> +++ b/hw/i2c/mpc_i2c.c
> @@ -24,6 +24,7 @@
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
>   #include "qom/object.h"
> +#include "trace.h"
>   
>   /* #define DEBUG_I2C */
>   
> @@ -224,8 +225,8 @@ static uint64_t mpc_i2c_read(void *opaque, hwaddr addr, unsigned size)
>           break;
>       }
>   
> -    DPRINTF("%s: addr " HWADDR_FMT_plx " %02" PRIx32 "\n", __func__,
> -                                         addr, value);
> +    trace_mpc_i2c_read(addr, value);
> +
>       return (uint64_t)value;
>   }
>   
> @@ -234,8 +235,8 @@ static void mpc_i2c_write(void *opaque, hwaddr addr,
>   {
>       MPCI2CState *s = opaque;
>   
> -    DPRINTF("%s: addr " HWADDR_FMT_plx " val %08" PRIx64 "\n", __func__,
> -                                             addr, value);
> +    trace_mpc_i2c_write(addr, value);
> +
>       switch (addr) {
>       case MPC_I2C_ADR:
>           s->adr = value & CADR_MASK;
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 6900e06eda..f708a7ace1 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -35,6 +35,11 @@ aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t va
>   aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
>   aspeed_i2c_bus_recv(const char *mode, int i, int count, uint8_t byte) "%s recv %d/%d 0x%02x"
>   
> +# mpc_i2c.c
> +
> +mpc_i2c_read(uint64_t addr, uint32_t value) "[0x%" PRIx64 "] -> 0x%02" PRIx32
> +mpc_i2c_write(uint64_t addr, uint32_t value) "[0x%" PRIx64 "] <- 0x%02" PRIx32
> +
>   # npcm7xx_smbus.c
>   
>   npcm7xx_smbus_read(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"

[PATCH 11/23] hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i2c/mpc_i2c.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
index 3d79c15653..16f4309ea9 100644
--- a/hw/i2c/mpc_i2c.c
+++ b/hw/i2c/mpc_i2c.c
@@ -20,7 +20,6 @@
 #include "qemu/osdep.h"
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
-#include "qemu/module.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "qom/object.h"
@@ -345,16 +344,13 @@ static void mpc_i2c_class_init(ObjectClass *klass, void *data)
     dc->desc = "MPC I2C Controller";
 }
 
-static const TypeInfo mpc_i2c_type_info = {
-    .name          = TYPE_MPC_I2C,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(MPCI2CState),
-    .class_init    = mpc_i2c_class_init,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_MPC_I2C,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(MPCI2CState),
+        .class_init    = mpc_i2c_class_init,
+    },
 };
 
-static void mpc_i2c_register_types(void)
-{
-    type_register_static(&mpc_i2c_type_info);
-}
-
-type_init(mpc_i2c_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 11/23] hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/i2c/mpc_i2c.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
> index 3d79c15653..16f4309ea9 100644
> --- a/hw/i2c/mpc_i2c.c
> +++ b/hw/i2c/mpc_i2c.c
> @@ -20,7 +20,6 @@
> #include "qemu/osdep.h"
> #include "hw/i2c/i2c.h"
> #include "hw/irq.h"
> -#include "qemu/module.h"
> #include "hw/sysbus.h"
> #include "migration/vmstate.h"
> #include "qom/object.h"
> @@ -345,16 +344,13 @@ static void mpc_i2c_class_init(ObjectClass *klass, void *data)
>     dc->desc = "MPC I2C Controller";
> }
>
> -static const TypeInfo mpc_i2c_type_info = {
> -    .name          = TYPE_MPC_I2C,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(MPCI2CState),
> -    .class_init    = mpc_i2c_class_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_MPC_I2C,
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(MPCI2CState),
> +        .class_init    = mpc_i2c_class_init,
> +    },
> };
>
> -static void mpc_i2c_register_types(void)
> -{
> -    type_register_static(&mpc_i2c_type_info);
> -}
> -
> -type_init(mpc_i2c_register_types)
> +DEFINE_TYPES(types)

What's the advantage of this when we have a single device? For these 
devices this looks like just code churn to me.

Regards,
BALATON Zoltan

Re: [PATCH 11/23] hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Am 23. September 2024 10:49:53 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/i2c/mpc_i2c.c | 20 ++++++++------------
>> 1 file changed, 8 insertions(+), 12 deletions(-)
>> 
>> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
>> index 3d79c15653..16f4309ea9 100644
>> --- a/hw/i2c/mpc_i2c.c
>> +++ b/hw/i2c/mpc_i2c.c
>> @@ -20,7 +20,6 @@
>> #include "qemu/osdep.h"
>> #include "hw/i2c/i2c.h"
>> #include "hw/irq.h"
>> -#include "qemu/module.h"
>> #include "hw/sysbus.h"
>> #include "migration/vmstate.h"
>> #include "qom/object.h"
>> @@ -345,16 +344,13 @@ static void mpc_i2c_class_init(ObjectClass *klass, void *data)
>>     dc->desc = "MPC I2C Controller";
>> }
>> 
>> -static const TypeInfo mpc_i2c_type_info = {
>> -    .name          = TYPE_MPC_I2C,
>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>> -    .instance_size = sizeof(MPCI2CState),
>> -    .class_init    = mpc_i2c_class_init,
>> +static const TypeInfo types[] = {
>> +    {
>> +        .name          = TYPE_MPC_I2C,
>> +        .parent        = TYPE_SYS_BUS_DEVICE,
>> +        .instance_size = sizeof(MPCI2CState),
>> +        .class_init    = mpc_i2c_class_init,
>> +    },
>> };
>> 
>> -static void mpc_i2c_register_types(void)
>> -{
>> -    type_register_static(&mpc_i2c_type_info);
>> -}
>> -
>> -type_init(mpc_i2c_register_types)
>> +DEFINE_TYPES(types)
>
>What's the advantage of this when we have a single device? For these devices this looks like just code churn to me.

It is still shorter and also more modern style. As a nice side effect it also helps in my experimental branch (which may never ship).

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

Re: [PATCH 11/23] hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Am 23. September 2024 10:49:53 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/i2c/mpc_i2c.c | 20 ++++++++------------
>>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
>>> index 3d79c15653..16f4309ea9 100644
>>> --- a/hw/i2c/mpc_i2c.c
>>> +++ b/hw/i2c/mpc_i2c.c
>>> @@ -20,7 +20,6 @@
>>> #include "qemu/osdep.h"
>>> #include "hw/i2c/i2c.h"
>>> #include "hw/irq.h"
>>> -#include "qemu/module.h"
>>> #include "hw/sysbus.h"
>>> #include "migration/vmstate.h"
>>> #include "qom/object.h"
>>> @@ -345,16 +344,13 @@ static void mpc_i2c_class_init(ObjectClass *klass, void *data)
>>>     dc->desc = "MPC I2C Controller";
>>> }
>>>
>>> -static const TypeInfo mpc_i2c_type_info = {
>>> -    .name          = TYPE_MPC_I2C,
>>> -    .parent        = TYPE_SYS_BUS_DEVICE,
>>> -    .instance_size = sizeof(MPCI2CState),
>>> -    .class_init    = mpc_i2c_class_init,
>>> +static const TypeInfo types[] = {
>>> +    {
>>> +        .name          = TYPE_MPC_I2C,
>>> +        .parent        = TYPE_SYS_BUS_DEVICE,
>>> +        .instance_size = sizeof(MPCI2CState),
>>> +        .class_init    = mpc_i2c_class_init,
>>> +    },
>>> };
>>>
>>> -static void mpc_i2c_register_types(void)
>>> -{
>>> -    type_register_static(&mpc_i2c_type_info);
>>> -}
>>> -
>>> -type_init(mpc_i2c_register_types)
>>> +DEFINE_TYPES(types)
>>
>> What's the advantage of this when we have a single device? For these devices this looks like just code churn to me.
>
> It is still shorter and also more modern style. As a nice side effect it also helps in my experimental branch (which may never ship).

I don't mind changing this but I see no real advantage either. It removes 
a one line function but adds a one element array instead which is about 
the same level of boilerplate and not less confusing for new people so it 
does not seem to help much.

Regards,
BALATON Zoltan

Re: [PATCH 11/23] hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/i2c/mpc_i2c.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
> index 3d79c15653..16f4309ea9 100644
> --- a/hw/i2c/mpc_i2c.c
> +++ b/hw/i2c/mpc_i2c.c
> @@ -20,7 +20,6 @@
>   #include "qemu/osdep.h"
>   #include "hw/i2c/i2c.h"
>   #include "hw/irq.h"
> -#include "qemu/module.h"
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
>   #include "qom/object.h"
> @@ -345,16 +344,13 @@ static void mpc_i2c_class_init(ObjectClass *klass, void *data)
>       dc->desc = "MPC I2C Controller";
>   }
>   
> -static const TypeInfo mpc_i2c_type_info = {
> -    .name          = TYPE_MPC_I2C,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(MPCI2CState),
> -    .class_init    = mpc_i2c_class_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_MPC_I2C,
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(MPCI2CState),
> +        .class_init    = mpc_i2c_class_init,
> +    },
>   };
>   
> -static void mpc_i2c_register_types(void)
> -{
> -    type_register_static(&mpc_i2c_type_info);
> -}
> -
> -type_init(mpc_i2c_register_types)
> +DEFINE_TYPES(types)

[PATCH 12/23] hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define

[Bernhard Beschow]

Prefer a macro rather than a string literal when instantiaging device models.

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

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index 97e5d47cec..d7ff2ba778 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -475,7 +475,7 @@ static void e500_pcihost_realize(DeviceState *dev, Error **errp)
     address_space_init(&s->bm_as, &s->bm, "pci-bm");
     pci_setup_iommu(b, &ppce500_iommu_ops, s);
 
-    pci_create_simple(b, 0, "e500-host-bridge");
+    pci_create_simple(b, 0, TYPE_PPC_E500_PCI_BRIDGE);
 
     memory_region_init(&s->container, OBJECT(h), "pci-container", PCIE500_ALL_SIZE);
     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, h,
-- 
2.46.1

Re: [PATCH 12/23] hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define

[BALATON Zoltan]

On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Prefer a macro rather than a string literal when instantiaging device models.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/pci-host/ppce500.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index 97e5d47cec..d7ff2ba778 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -475,7 +475,7 @@ static void e500_pcihost_realize(DeviceState *dev, Error **errp)
>     address_space_init(&s->bm_as, &s->bm, "pci-bm");
>     pci_setup_iommu(b, &ppce500_iommu_ops, s);
>
> -    pci_create_simple(b, 0, "e500-host-bridge");
> +    pci_create_simple(b, 0, TYPE_PPC_E500_PCI_BRIDGE);
>
>     memory_region_init(&s->container, OBJECT(h), "pci-container", PCIE500_ALL_SIZE);
>     memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_be_ops, h,
>

[PATCH 13/23] hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/ppce500.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index d7ff2ba778..1ce79ea20c 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -21,7 +21,6 @@
 #include "hw/pci/pci_device.h"
 #include "hw/pci/pci_host.h"
 #include "qemu/bswap.h"
-#include "qemu/module.h"
 #include "hw/pci-host/ppce500.h"
 #include "qom/object.h"
 
@@ -508,17 +507,6 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = false;
 }
 
-static const TypeInfo e500_host_bridge_info = {
-    .name          = TYPE_PPC_E500_PCI_BRIDGE,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PPCE500PCIBridgeState),
-    .class_init    = e500_host_bridge_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
-    },
-};
-
 static Property pcihost_properties[] = {
     DEFINE_PROP_UINT32("first_slot", PPCE500PCIState, first_slot, 0x11),
     DEFINE_PROP_UINT32("first_pin_irq", PPCE500PCIState, first_pin_irq, 0x1),
@@ -535,17 +523,23 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_ppce500_pci;
 }
 
-static const TypeInfo e500_pcihost_info = {
-    .name          = TYPE_PPC_E500_PCI_HOST_BRIDGE,
-    .parent        = TYPE_PCI_HOST_BRIDGE,
-    .instance_size = sizeof(PPCE500PCIState),
-    .class_init    = e500_pcihost_class_init,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_PPC_E500_PCI_BRIDGE,
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(PPCE500PCIBridgeState),
+        .class_init    = e500_host_bridge_class_init,
+        .interfaces    = (InterfaceInfo[]) {
+            { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+            { },
+        },
+    },
+    {
+        .name          = TYPE_PPC_E500_PCI_HOST_BRIDGE,
+        .parent        = TYPE_PCI_HOST_BRIDGE,
+        .instance_size = sizeof(PPCE500PCIState),
+        .class_init    = e500_pcihost_class_init,
+    },
 };
 
-static void e500_pci_register_types(void)
-{
-    type_register_static(&e500_pcihost_info);
-    type_register_static(&e500_host_bridge_info);
-}
-
-type_init(e500_pci_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 13/23] hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/pci-host/ppce500.c | 42 ++++++++++++++++++------------------------
>   1 file changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index d7ff2ba778..1ce79ea20c 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -21,7 +21,6 @@
>   #include "hw/pci/pci_device.h"
>   #include "hw/pci/pci_host.h"
>   #include "qemu/bswap.h"
> -#include "qemu/module.h"
>   #include "hw/pci-host/ppce500.h"
>   #include "qom/object.h"
>   
> @@ -508,17 +507,6 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
>       dc->user_creatable = false;
>   }
>   
> -static const TypeInfo e500_host_bridge_info = {
> -    .name          = TYPE_PPC_E500_PCI_BRIDGE,
> -    .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PPCE500PCIBridgeState),
> -    .class_init    = e500_host_bridge_class_init,
> -    .interfaces = (InterfaceInfo[]) {
> -        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> -        { },
> -    },
> -};
> -
>   static Property pcihost_properties[] = {
>       DEFINE_PROP_UINT32("first_slot", PPCE500PCIState, first_slot, 0x11),
>       DEFINE_PROP_UINT32("first_pin_irq", PPCE500PCIState, first_pin_irq, 0x1),
> @@ -535,17 +523,23 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
>       dc->vmsd = &vmstate_ppce500_pci;
>   }
>   
> -static const TypeInfo e500_pcihost_info = {
> -    .name          = TYPE_PPC_E500_PCI_HOST_BRIDGE,
> -    .parent        = TYPE_PCI_HOST_BRIDGE,
> -    .instance_size = sizeof(PPCE500PCIState),
> -    .class_init    = e500_pcihost_class_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_PPC_E500_PCI_BRIDGE,
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(PPCE500PCIBridgeState),
> +        .class_init    = e500_host_bridge_class_init,
> +        .interfaces    = (InterfaceInfo[]) {
> +            { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +            { },
> +        },
> +    },
> +    {
> +        .name          = TYPE_PPC_E500_PCI_HOST_BRIDGE,
> +        .parent        = TYPE_PCI_HOST_BRIDGE,
> +        .instance_size = sizeof(PPCE500PCIState),
> +        .class_init    = e500_pcihost_class_init,
> +    },
>   };
>   
> -static void e500_pci_register_types(void)
> -{
> -    type_register_static(&e500_pcihost_info);
> -    type_register_static(&e500_host_bridge_info);
> -}
> -
> -type_init(e500_pci_register_types)
> +DEFINE_TYPES(types)

[PATCH 14/23] hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/gpio/mpc8xxx.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/gpio/mpc8xxx.c b/hw/gpio/mpc8xxx.c
index 63b7a5c881..de183c3be5 100644
--- a/hw/gpio/mpc8xxx.c
+++ b/hw/gpio/mpc8xxx.c
@@ -23,7 +23,6 @@
 #include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
-#include "qemu/module.h"
 #include "qom/object.h"
 
 #define TYPE_MPC8XXX_GPIO "mpc8xxx_gpio"
@@ -208,17 +207,14 @@ static void mpc8xxx_gpio_class_init(ObjectClass *klass, void *data)
     device_class_set_legacy_reset(dc, mpc8xxx_gpio_reset);
 }
 
-static const TypeInfo mpc8xxx_gpio_info = {
-    .name          = TYPE_MPC8XXX_GPIO,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(MPC8XXXGPIOState),
-    .instance_init = mpc8xxx_gpio_initfn,
-    .class_init    = mpc8xxx_gpio_class_init,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_MPC8XXX_GPIO,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(MPC8XXXGPIOState),
+        .instance_init = mpc8xxx_gpio_initfn,
+        .class_init    = mpc8xxx_gpio_class_init,
+    },
 };
 
-static void mpc8xxx_gpio_register_types(void)
-{
-    type_register_static(&mpc8xxx_gpio_info);
-}
-
-type_init(mpc8xxx_gpio_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 14/23] hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/gpio/mpc8xxx.c | 22 +++++++++-------------
>   1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/gpio/mpc8xxx.c b/hw/gpio/mpc8xxx.c
> index 63b7a5c881..de183c3be5 100644
> --- a/hw/gpio/mpc8xxx.c
> +++ b/hw/gpio/mpc8xxx.c
> @@ -23,7 +23,6 @@
>   #include "hw/irq.h"
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
> -#include "qemu/module.h"
>   #include "qom/object.h"
>   
>   #define TYPE_MPC8XXX_GPIO "mpc8xxx_gpio"
> @@ -208,17 +207,14 @@ static void mpc8xxx_gpio_class_init(ObjectClass *klass, void *data)
>       device_class_set_legacy_reset(dc, mpc8xxx_gpio_reset);
>   }
>   
> -static const TypeInfo mpc8xxx_gpio_info = {
> -    .name          = TYPE_MPC8XXX_GPIO,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(MPC8XXXGPIOState),
> -    .instance_init = mpc8xxx_gpio_initfn,
> -    .class_init    = mpc8xxx_gpio_class_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_MPC8XXX_GPIO,
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(MPC8XXXGPIOState),
> +        .instance_init = mpc8xxx_gpio_initfn,
> +        .class_init    = mpc8xxx_gpio_class_init,
> +    },
>   };
>   
> -static void mpc8xxx_gpio_register_types(void)
> -{
> -    type_register_static(&mpc8xxx_gpio_info);
> -}
> -
> -type_init(mpc8xxx_gpio_register_types)
> +DEFINE_TYPES(types)

[PATCH 15/23] hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/mpc8544_guts.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
index 6688fd44c3..cbb1e3adda 100644
--- a/hw/ppc/mpc8544_guts.c
+++ b/hw/ppc/mpc8544_guts.c
@@ -18,7 +18,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "qemu/module.h"
 #include "qemu/log.h"
 #include "sysemu/runstate.h"
 #include "cpu.h"
@@ -141,16 +140,13 @@ static void mpc8544_guts_initfn(Object *obj)
     sysbus_init_mmio(d, &s->iomem);
 }
 
-static const TypeInfo mpc8544_guts_info = {
-    .name          = TYPE_MPC8544_GUTS,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(GutsState),
-    .instance_init = mpc8544_guts_initfn,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_MPC8544_GUTS,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(GutsState),
+        .instance_init = mpc8544_guts_initfn,
+    },
 };
 
-static void mpc8544_guts_register_types(void)
-{
-    type_register_static(&mpc8544_guts_info);
-}
-
-type_init(mpc8544_guts_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 15/23] hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/ppc/mpc8544_guts.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/mpc8544_guts.c b/hw/ppc/mpc8544_guts.c
> index 6688fd44c3..cbb1e3adda 100644
> --- a/hw/ppc/mpc8544_guts.c
> +++ b/hw/ppc/mpc8544_guts.c
> @@ -18,7 +18,6 @@
>    */
>   
>   #include "qemu/osdep.h"
> -#include "qemu/module.h"
>   #include "qemu/log.h"
>   #include "sysemu/runstate.h"
>   #include "cpu.h"
> @@ -141,16 +140,13 @@ static void mpc8544_guts_initfn(Object *obj)
>       sysbus_init_mmio(d, &s->iomem);
>   }
>   
> -static const TypeInfo mpc8544_guts_info = {
> -    .name          = TYPE_MPC8544_GUTS,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(GutsState),
> -    .instance_init = mpc8544_guts_initfn,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_MPC8544_GUTS,
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(GutsState),
> +        .instance_init = mpc8544_guts_initfn,
> +    },
>   };
>   
> -static void mpc8544_guts_register_types(void)
> -{
> -    type_register_static(&mpc8544_guts_info);
> -}
> -
> -type_init(mpc8544_guts_register_types)
> +DEFINE_TYPES(types)

[PATCH 16/23] hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/net/fsl_etsec/etsec.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 3fdd16ef2e..9bd886b996 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -36,7 +36,6 @@
 #include "registers.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
-#include "qemu/module.h"
 
 /* #define HEX_DUMP */
 /* #define DEBUG_REGISTER */
@@ -431,17 +430,14 @@ static void etsec_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = true;
 }
 
-static const TypeInfo etsec_info = {
-    .name                  = TYPE_ETSEC_COMMON,
-    .parent                = TYPE_SYS_BUS_DEVICE,
-    .instance_size         = sizeof(eTSEC),
-    .class_init            = etsec_class_init,
-    .instance_init         = etsec_instance_init,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_ETSEC_COMMON,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(eTSEC),
+        .class_init    = etsec_class_init,
+        .instance_init = etsec_instance_init,
+    },
 };
 
-static void etsec_register_types(void)
-{
-    type_register_static(&etsec_info);
-}
-
-type_init(etsec_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 16/23] hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   hw/net/fsl_etsec/etsec.c | 22 +++++++++-------------
>   1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 3fdd16ef2e..9bd886b996 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -36,7 +36,6 @@
>   #include "registers.h"
>   #include "qapi/error.h"
>   #include "qemu/log.h"
> -#include "qemu/module.h"
>   
>   /* #define HEX_DUMP */
>   /* #define DEBUG_REGISTER */
> @@ -431,17 +430,14 @@ static void etsec_class_init(ObjectClass *klass, void *data)
>       dc->user_creatable = true;
>   }
>   
> -static const TypeInfo etsec_info = {
> -    .name                  = TYPE_ETSEC_COMMON,
> -    .parent                = TYPE_SYS_BUS_DEVICE,
> -    .instance_size         = sizeof(eTSEC),
> -    .class_init            = etsec_class_init,
> -    .instance_init         = etsec_instance_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_ETSEC_COMMON,
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(eTSEC),
> +        .class_init    = etsec_class_init,
> +        .instance_init = etsec_instance_init,
> +    },
>   };
>   
> -static void etsec_register_types(void)
> -{
> -    type_register_static(&etsec_info);
> -}
> -
> -type_init(etsec_register_types)
> +DEFINE_TYPES(types)

[PATCH 17/23] hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch

[Bernhard Beschow]

Allows to clearly mark code sections relying on this device type.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c       | 2 +-
 hw/intc/Kconfig     | 4 ++++
 hw/intc/meson.build | 3 +--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 4ee4304a8a..149e608324 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -828,7 +828,7 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500MachineState *pms,
 static DeviceState *ppce500_init_mpic_kvm(const PPCE500MachineClass *pmc,
                                           Error **errp)
 {
-#ifdef CONFIG_KVM
+#ifdef CONFIG_OPENPIC_KVM
     DeviceState *dev;
     CPUState *cs;
 
diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index dd405bdb5d..a3df98ae59 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -16,6 +16,10 @@ config OPENPIC
     bool
     select MSI_NONBROKEN
 
+config OPENPIC_KVM
+    bool
+    depends on OPENPIC && KVM
+
 config APIC
     bool
     select MSI_NONBROKEN
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index 6bfdc4eb33..b9de6bf5c6 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -48,8 +48,7 @@ specific_ss.add(when: 'CONFIG_IOAPIC', if_true: files('ioapic.c'))
 specific_ss.add(when: 'CONFIG_LOONGSON_LIOINTC', if_true: files('loongson_liointc.c'))
 specific_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_gic.c'))
 specific_ss.add(when: 'CONFIG_OMPIC', if_true: files('ompic.c'))
-specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_OPENPIC'],
-		if_true: files('openpic_kvm.c'))
+specific_ss.add(when: 'CONFIG_OPENPIC_KVM', if_true: files('openpic_kvm.c'))
 specific_ss.add(when: 'CONFIG_POWERNV', if_true: files('xics_pnv.c', 'pnv_xive.c', 'pnv_xive2.c'))
 specific_ss.add(when: 'CONFIG_PPC_UIC', if_true: files('ppc-uic.c'))
 specific_ss.add(when: 'CONFIG_RX_ICU', if_true: files('rx_icu.c'))
-- 
2.46.1

Re: [PATCH 17/23] hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Allows to clearly mark code sections relying on this device type.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   hw/ppc/e500.c       | 2 +-
>   hw/intc/Kconfig     | 4 ++++
>   hw/intc/meson.build | 3 +--
>   3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 4ee4304a8a..149e608324 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -828,7 +828,7 @@ static DeviceState *ppce500_init_mpic_qemu(PPCE500MachineState *pms,
>   static DeviceState *ppce500_init_mpic_kvm(const PPCE500MachineClass *pmc,
>                                             Error **errp)
>   {
> -#ifdef CONFIG_KVM
> +#ifdef CONFIG_OPENPIC_KVM
>       DeviceState *dev;
>       CPUState *cs;
>   
> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
> index dd405bdb5d..a3df98ae59 100644
> --- a/hw/intc/Kconfig
> +++ b/hw/intc/Kconfig
> @@ -16,6 +16,10 @@ config OPENPIC
>       bool
>       select MSI_NONBROKEN
>   
> +config OPENPIC_KVM
> +    bool
> +    depends on OPENPIC && KVM
> +
>   config APIC
>       bool
>       select MSI_NONBROKEN
> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index 6bfdc4eb33..b9de6bf5c6 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -48,8 +48,7 @@ specific_ss.add(when: 'CONFIG_IOAPIC', if_true: files('ioapic.c'))
>   specific_ss.add(when: 'CONFIG_LOONGSON_LIOINTC', if_true: files('loongson_liointc.c'))
>   specific_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_gic.c'))
>   specific_ss.add(when: 'CONFIG_OMPIC', if_true: files('ompic.c'))
> -specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_OPENPIC'],
> -		if_true: files('openpic_kvm.c'))
> +specific_ss.add(when: 'CONFIG_OPENPIC_KVM', if_true: files('openpic_kvm.c'))
>   specific_ss.add(when: 'CONFIG_POWERNV', if_true: files('xics_pnv.c', 'pnv_xive.c', 'pnv_xive2.c'))
>   specific_ss.add(when: 'CONFIG_PPC_UIC', if_true: files('ppc-uic.c'))
>   specific_ss.add(when: 'CONFIG_RX_ICU', if_true: files('rx_icu.c'))

[PATCH 18/23] hw/sd/sdhci: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/sd/sdhci.c | 62 +++++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 87122e4245..3ed2d8658a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -37,7 +37,6 @@
 #include "migration/vmstate.h"
 #include "sdhci-internal.h"
 #include "qemu/log.h"
-#include "qemu/module.h"
 #include "trace.h"
 #include "qom/object.h"
 
@@ -1598,15 +1597,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
     sdhci_common_class_init(klass, data);
 }
 
-static const TypeInfo sdhci_sysbus_info = {
-    .name = TYPE_SYSBUS_SDHCI,
-    .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(SDHCIState),
-    .instance_init = sdhci_sysbus_init,
-    .instance_finalize = sdhci_sysbus_finalize,
-    .class_init = sdhci_sysbus_class_init,
-};
-
 /* --- qdev bus master --- */
 
 static void sdhci_bus_class_init(ObjectClass *klass, void *data)
@@ -1617,13 +1607,6 @@ static void sdhci_bus_class_init(ObjectClass *klass, void *data)
     sbc->set_readonly = sdhci_set_readonly;
 }
 
-static const TypeInfo sdhci_bus_info = {
-    .name = TYPE_SDHCI_BUS,
-    .parent = TYPE_SD_BUS,
-    .instance_size = sizeof(SDBus),
-    .class_init = sdhci_bus_class_init,
-};
-
 /* --- qdev i.MX eSDHC --- */
 
 #define USDHC_MIX_CTRL                  0x48
@@ -1882,12 +1865,6 @@ static void imx_usdhc_init(Object *obj)
     s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
 }
 
-static const TypeInfo imx_usdhc_info = {
-    .name = TYPE_IMX_USDHC,
-    .parent = TYPE_SYSBUS_SDHCI,
-    .instance_init = imx_usdhc_init,
-};
-
 /* --- qdev Samsung s3c --- */
 
 #define S3C_SDHCI_CONTROL2      0x80
@@ -1946,18 +1923,31 @@ static void sdhci_s3c_init(Object *obj)
     s->io_ops = &sdhci_s3c_mmio_ops;
 }
 
-static const TypeInfo sdhci_s3c_info = {
-    .name = TYPE_S3C_SDHCI  ,
-    .parent = TYPE_SYSBUS_SDHCI,
-    .instance_init = sdhci_s3c_init,
+static const TypeInfo types[] = {
+    {
+        .name = TYPE_SDHCI_BUS,
+        .parent = TYPE_SD_BUS,
+        .instance_size = sizeof(SDBus),
+        .class_init = sdhci_bus_class_init,
+    },
+    {
+        .name = TYPE_SYSBUS_SDHCI,
+        .parent = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(SDHCIState),
+        .instance_init = sdhci_sysbus_init,
+        .instance_finalize = sdhci_sysbus_finalize,
+        .class_init = sdhci_sysbus_class_init,
+    },
+    {
+        .name = TYPE_IMX_USDHC,
+        .parent = TYPE_SYSBUS_SDHCI,
+        .instance_init = imx_usdhc_init,
+    },
+    {
+        .name = TYPE_S3C_SDHCI,
+        .parent = TYPE_SYSBUS_SDHCI,
+        .instance_init = sdhci_s3c_init,
+    },
 };
 
-static void sdhci_register_types(void)
-{
-    type_register_static(&sdhci_sysbus_info);
-    type_register_static(&sdhci_bus_info);
-    type_register_static(&imx_usdhc_info);
-    type_register_static(&sdhci_s3c_info);
-}
-
-type_init(sdhci_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 18/23] hw/sd/sdhci: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   hw/sd/sdhci.c | 62 +++++++++++++++++++++------------------------------
>   1 file changed, 26 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 87122e4245..3ed2d8658a 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -37,7 +37,6 @@
>   #include "migration/vmstate.h"
>   #include "sdhci-internal.h"
>   #include "qemu/log.h"
> -#include "qemu/module.h"
>   #include "trace.h"
>   #include "qom/object.h"
>   
> @@ -1598,15 +1597,6 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
>       sdhci_common_class_init(klass, data);
>   }
>   
> -static const TypeInfo sdhci_sysbus_info = {
> -    .name = TYPE_SYSBUS_SDHCI,
> -    .parent = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(SDHCIState),
> -    .instance_init = sdhci_sysbus_init,
> -    .instance_finalize = sdhci_sysbus_finalize,
> -    .class_init = sdhci_sysbus_class_init,
> -};
> -
>   /* --- qdev bus master --- */
>   
>   static void sdhci_bus_class_init(ObjectClass *klass, void *data)
> @@ -1617,13 +1607,6 @@ static void sdhci_bus_class_init(ObjectClass *klass, void *data)
>       sbc->set_readonly = sdhci_set_readonly;
>   }
>   
> -static const TypeInfo sdhci_bus_info = {
> -    .name = TYPE_SDHCI_BUS,
> -    .parent = TYPE_SD_BUS,
> -    .instance_size = sizeof(SDBus),
> -    .class_init = sdhci_bus_class_init,
> -};
> -
>   /* --- qdev i.MX eSDHC --- */
>   
>   #define USDHC_MIX_CTRL                  0x48
> @@ -1882,12 +1865,6 @@ static void imx_usdhc_init(Object *obj)
>       s->quirks = SDHCI_QUIRK_NO_BUSY_IRQ;
>   }
>   
> -static const TypeInfo imx_usdhc_info = {
> -    .name = TYPE_IMX_USDHC,
> -    .parent = TYPE_SYSBUS_SDHCI,
> -    .instance_init = imx_usdhc_init,
> -};
> -
>   /* --- qdev Samsung s3c --- */
>   
>   #define S3C_SDHCI_CONTROL2      0x80
> @@ -1946,18 +1923,31 @@ static void sdhci_s3c_init(Object *obj)
>       s->io_ops = &sdhci_s3c_mmio_ops;
>   }
>   
> -static const TypeInfo sdhci_s3c_info = {
> -    .name = TYPE_S3C_SDHCI  ,
> -    .parent = TYPE_SYSBUS_SDHCI,
> -    .instance_init = sdhci_s3c_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name = TYPE_SDHCI_BUS,
> +        .parent = TYPE_SD_BUS,
> +        .instance_size = sizeof(SDBus),
> +        .class_init = sdhci_bus_class_init,
> +    },
> +    {
> +        .name = TYPE_SYSBUS_SDHCI,
> +        .parent = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(SDHCIState),
> +        .instance_init = sdhci_sysbus_init,
> +        .instance_finalize = sdhci_sysbus_finalize,
> +        .class_init = sdhci_sysbus_class_init,
> +    },
> +    {
> +        .name = TYPE_IMX_USDHC,
> +        .parent = TYPE_SYSBUS_SDHCI,
> +        .instance_init = imx_usdhc_init,
> +    },
> +    {
> +        .name = TYPE_S3C_SDHCI,
> +        .parent = TYPE_SYSBUS_SDHCI,
> +        .instance_init = sdhci_s3c_init,
> +    },
>   };
>   
> -static void sdhci_register_types(void)
> -{
> -    type_register_static(&sdhci_sysbus_info);
> -    type_register_static(&sdhci_bus_info);
> -    type_register_static(&imx_usdhc_info);
> -    type_register_static(&sdhci_s3c_info);
> -}
> -
> -type_init(sdhci_register_types)
> +DEFINE_TYPES(types)

[PATCH 19/23] hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/block/pflash_cfi01.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 7b6ec64442..cf11dada29 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -47,7 +47,6 @@
 #include "qemu/bitops.h"
 #include "qemu/host-utils.h"
 #include "qemu/log.h"
-#include "qemu/module.h"
 #include "qemu/option.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
@@ -947,20 +946,16 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
-
-static const TypeInfo pflash_cfi01_info = {
-    .name           = TYPE_PFLASH_CFI01,
-    .parent         = TYPE_SYS_BUS_DEVICE,
-    .instance_size  = sizeof(PFlashCFI01),
-    .class_init     = pflash_cfi01_class_init,
+static const TypeInfo types[] = {
+    {
+        .name           = TYPE_PFLASH_CFI01,
+        .parent         = TYPE_SYS_BUS_DEVICE,
+        .instance_size  = sizeof(PFlashCFI01),
+        .class_init     = pflash_cfi01_class_init,
+    },
 };
 
-static void pflash_cfi01_register_types(void)
-{
-    type_register_static(&pflash_cfi01_info);
-}
-
-type_init(pflash_cfi01_register_types)
+DEFINE_TYPES(types)
 
 PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    const char *name,
-- 
2.46.1

Re: [PATCH 19/23] hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/block/pflash_cfi01.c | 21 ++++++++-------------
>   1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 7b6ec64442..cf11dada29 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -47,7 +47,6 @@
>   #include "qemu/bitops.h"
>   #include "qemu/host-utils.h"
>   #include "qemu/log.h"
> -#include "qemu/module.h"
>   #include "qemu/option.h"
>   #include "hw/sysbus.h"
>   #include "migration/vmstate.h"
> @@ -947,20 +946,16 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>   }
>   
> -
> -static const TypeInfo pflash_cfi01_info = {
> -    .name           = TYPE_PFLASH_CFI01,
> -    .parent         = TYPE_SYS_BUS_DEVICE,
> -    .instance_size  = sizeof(PFlashCFI01),
> -    .class_init     = pflash_cfi01_class_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name           = TYPE_PFLASH_CFI01,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(PFlashCFI01),
> +        .class_init     = pflash_cfi01_class_init,
> +    },
>   };
>   
> -static void pflash_cfi01_register_types(void)
> -{
> -    type_register_static(&pflash_cfi01_info);
> -}
> -
> -type_init(pflash_cfi01_register_types)
> +DEFINE_TYPES(types)
>   
>   PFlashCFI01 *pflash_cfi01_register(hwaddr base,
>                                      const char *name,

[PATCH 20/23] hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i2c/smbus_eeprom.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 9e62c27a1a..1d4d9704bf 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -151,19 +151,16 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     dc->user_creatable = false;
 }
 
-static const TypeInfo smbus_eeprom_info = {
-    .name          = TYPE_SMBUS_EEPROM,
-    .parent        = TYPE_SMBUS_DEVICE,
-    .instance_size = sizeof(SMBusEEPROMDevice),
-    .class_init    = smbus_eeprom_class_initfn,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_SMBUS_EEPROM,
+        .parent        = TYPE_SMBUS_DEVICE,
+        .instance_size = sizeof(SMBusEEPROMDevice),
+        .class_init    = smbus_eeprom_class_initfn,
+    },
 };
 
-static void smbus_eeprom_register_types(void)
-{
-    type_register_static(&smbus_eeprom_info);
-}
-
-type_init(smbus_eeprom_register_types)
+DEFINE_TYPES(types)
 
 void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
 {
-- 
2.46.1

Re: [PATCH 20/23] hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   hw/i2c/smbus_eeprom.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 9e62c27a1a..1d4d9704bf 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -151,19 +151,16 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>       dc->user_creatable = false;
>   }
>   
> -static const TypeInfo smbus_eeprom_info = {
> -    .name          = TYPE_SMBUS_EEPROM,
> -    .parent        = TYPE_SMBUS_DEVICE,
> -    .instance_size = sizeof(SMBusEEPROMDevice),
> -    .class_init    = smbus_eeprom_class_initfn,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_SMBUS_EEPROM,
> +        .parent        = TYPE_SMBUS_DEVICE,
> +        .instance_size = sizeof(SMBusEEPROMDevice),
> +        .class_init    = smbus_eeprom_class_initfn,
> +    },
>   };
>   
> -static void smbus_eeprom_register_types(void)
> -{
> -    type_register_static(&smbus_eeprom_info);
> -}
> -
> -type_init(smbus_eeprom_register_types)
> +DEFINE_TYPES(types)
>   
>   void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
>   {

[PATCH 21/23] hw/rtc/ds1338: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

---
 hw/rtc/ds1338.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/hw/rtc/ds1338.c b/hw/rtc/ds1338.c
index a5fe221418..6de13caf99 100644
--- a/hw/rtc/ds1338.c
+++ b/hw/rtc/ds1338.c
@@ -14,7 +14,6 @@
 #include "hw/i2c/i2c.h"
 #include "migration/vmstate.h"
 #include "qemu/bcd.h"
-#include "qemu/module.h"
 #include "qom/object.h"
 #include "sysemu/rtc.h"
 
@@ -227,16 +226,13 @@ static void ds1338_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_ds1338;
 }
 
-static const TypeInfo ds1338_info = {
-    .name          = TYPE_DS1338,
-    .parent        = TYPE_I2C_SLAVE,
-    .instance_size = sizeof(DS1338State),
-    .class_init    = ds1338_class_init,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_DS1338,
+        .parent        = TYPE_I2C_SLAVE,
+        .instance_size = sizeof(DS1338State),
+        .class_init    = ds1338_class_init,
+    },
 };
 
-static void ds1338_register_types(void)
-{
-    type_register_static(&ds1338_info);
-}
-
-type_init(ds1338_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 21/23] hw/rtc/ds1338: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

Sob is missing.


Thanks,

C.

On 9/23/24 11:30, Bernhard Beschow wrote:
> ---
>   hw/rtc/ds1338.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/rtc/ds1338.c b/hw/rtc/ds1338.c
> index a5fe221418..6de13caf99 100644
> --- a/hw/rtc/ds1338.c
> +++ b/hw/rtc/ds1338.c
> @@ -14,7 +14,6 @@
>   #include "hw/i2c/i2c.h"
>   #include "migration/vmstate.h"
>   #include "qemu/bcd.h"
> -#include "qemu/module.h"
>   #include "qom/object.h"
>   #include "sysemu/rtc.h"
>   
> @@ -227,16 +226,13 @@ static void ds1338_class_init(ObjectClass *klass, void *data)
>       dc->vmsd = &vmstate_ds1338;
>   }
>   
> -static const TypeInfo ds1338_info = {
> -    .name          = TYPE_DS1338,
> -    .parent        = TYPE_I2C_SLAVE,
> -    .instance_size = sizeof(DS1338State),
> -    .class_init    = ds1338_class_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_DS1338,
> +        .parent        = TYPE_I2C_SLAVE,
> +        .instance_size = sizeof(DS1338State),
> +        .class_init    = ds1338_class_init,
> +    },
>   };
>   
> -static void ds1338_register_types(void)
> -{
> -    type_register_static(&ds1338_info);
> -}
> -
> -type_init(ds1338_register_types)
> +DEFINE_TYPES(types)

[PATCH 22/23] hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/usb/hcd-ehci-sysbus.c | 118 +++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 68 deletions(-)

diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index 2b1652f7a8..87a3bebe3e 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -19,7 +19,6 @@
 #include "hw/qdev-properties.h"
 #include "hw/usb/hcd-ehci.h"
 #include "migration/vmstate.h"
-#include "qemu/module.h"
 
 static const VMStateDescription vmstate_ehci_sysbus = {
     .name        = "ehci-sysbus",
@@ -97,17 +96,6 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_type_info = {
-    .name          = TYPE_SYS_BUS_EHCI,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(EHCISysBusState),
-    .instance_init = ehci_sysbus_init,
-    .instance_finalize = ehci_sysbus_finalize,
-    .abstract      = true,
-    .class_init    = ehci_sysbus_class_init,
-    .class_size    = sizeof(SysBusEHCIClass),
-};
-
 static void ehci_platform_class_init(ObjectClass *oc, void *data)
 {
     SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -118,12 +106,6 @@ static void ehci_platform_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_platform_type_info = {
-    .name          = TYPE_PLATFORM_EHCI,
-    .parent        = TYPE_SYS_BUS_EHCI,
-    .class_init    = ehci_platform_class_init,
-};
-
 static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
 {
     SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -134,12 +116,6 @@ static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_exynos4210_type_info = {
-    .name          = TYPE_EXYNOS4210_EHCI,
-    .parent        = TYPE_SYS_BUS_EHCI,
-    .class_init    = ehci_exynos4210_class_init,
-};
-
 static void ehci_aw_h3_class_init(ObjectClass *oc, void *data)
 {
     SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -150,12 +126,6 @@ static void ehci_aw_h3_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_aw_h3_type_info = {
-    .name          = TYPE_AW_H3_EHCI,
-    .parent        = TYPE_SYS_BUS_EHCI,
-    .class_init    = ehci_aw_h3_class_init,
-};
-
 static void ehci_npcm7xx_class_init(ObjectClass *oc, void *data)
 {
     SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -168,12 +138,6 @@ static void ehci_npcm7xx_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_npcm7xx_type_info = {
-    .name          = TYPE_NPCM7XX_EHCI,
-    .parent        = TYPE_SYS_BUS_EHCI,
-    .class_init    = ehci_npcm7xx_class_init,
-};
-
 static void ehci_tegra2_class_init(ObjectClass *oc, void *data)
 {
     SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -184,12 +148,6 @@ static void ehci_tegra2_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_tegra2_type_info = {
-    .name          = TYPE_TEGRA2_EHCI,
-    .parent        = TYPE_SYS_BUS_EHCI,
-    .class_init    = ehci_tegra2_class_init,
-};
-
 static void ehci_ppc4xx_init(Object *o)
 {
     EHCISysBusState *s = SYS_BUS_EHCI(o);
@@ -207,13 +165,6 @@ static void ehci_ppc4xx_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_ppc4xx_type_info = {
-    .name          = TYPE_PPC4xx_EHCI,
-    .parent        = TYPE_SYS_BUS_EHCI,
-    .class_init    = ehci_ppc4xx_class_init,
-    .instance_init = ehci_ppc4xx_init,
-};
-
 /*
  * Faraday FUSBH200 USB 2.0 EHCI
  */
@@ -282,24 +233,55 @@ static void fusbh200_ehci_class_init(ObjectClass *oc, void *data)
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
-static const TypeInfo ehci_fusbh200_type_info = {
-    .name          = TYPE_FUSBH200_EHCI,
-    .parent        = TYPE_SYS_BUS_EHCI,
-    .instance_size = sizeof(FUSBH200EHCIState),
-    .instance_init = fusbh200_ehci_init,
-    .class_init    = fusbh200_ehci_class_init,
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_SYS_BUS_EHCI,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(EHCISysBusState),
+        .instance_init = ehci_sysbus_init,
+        .instance_finalize = ehci_sysbus_finalize,
+        .abstract      = true,
+        .class_init    = ehci_sysbus_class_init,
+        .class_size    = sizeof(SysBusEHCIClass),
+    },
+    {
+        .name          = TYPE_PLATFORM_EHCI,
+        .parent        = TYPE_SYS_BUS_EHCI,
+        .class_init    = ehci_platform_class_init,
+    },
+    {
+        .name          = TYPE_EXYNOS4210_EHCI,
+        .parent        = TYPE_SYS_BUS_EHCI,
+        .class_init    = ehci_exynos4210_class_init,
+    },
+    {
+        .name          = TYPE_AW_H3_EHCI,
+        .parent        = TYPE_SYS_BUS_EHCI,
+        .class_init    = ehci_aw_h3_class_init,
+    },
+    {
+        .name          = TYPE_NPCM7XX_EHCI,
+        .parent        = TYPE_SYS_BUS_EHCI,
+        .class_init    = ehci_npcm7xx_class_init,
+    },
+    {
+        .name          = TYPE_TEGRA2_EHCI,
+        .parent        = TYPE_SYS_BUS_EHCI,
+        .class_init    = ehci_tegra2_class_init,
+    },
+    {
+        .name          = TYPE_PPC4xx_EHCI,
+        .parent        = TYPE_SYS_BUS_EHCI,
+        .class_init    = ehci_ppc4xx_class_init,
+        .instance_init = ehci_ppc4xx_init,
+    },
+    {
+        .name          = TYPE_FUSBH200_EHCI,
+        .parent        = TYPE_SYS_BUS_EHCI,
+        .instance_size = sizeof(FUSBH200EHCIState),
+        .instance_init = fusbh200_ehci_init,
+        .class_init    = fusbh200_ehci_class_init,
+    },
 };
 
-static void ehci_sysbus_register_types(void)
-{
-    type_register_static(&ehci_type_info);
-    type_register_static(&ehci_platform_type_info);
-    type_register_static(&ehci_exynos4210_type_info);
-    type_register_static(&ehci_aw_h3_type_info);
-    type_register_static(&ehci_npcm7xx_type_info);
-    type_register_static(&ehci_tegra2_type_info);
-    type_register_static(&ehci_ppc4xx_type_info);
-    type_register_static(&ehci_fusbh200_type_info);
-}
-
-type_init(ehci_sysbus_register_types)
+DEFINE_TYPES(types)
-- 
2.46.1

Re: [PATCH 22/23] hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/usb/hcd-ehci-sysbus.c | 118 +++++++++++++++++----------------------
>   1 file changed, 50 insertions(+), 68 deletions(-)
> 
> diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
> index 2b1652f7a8..87a3bebe3e 100644
> --- a/hw/usb/hcd-ehci-sysbus.c
> +++ b/hw/usb/hcd-ehci-sysbus.c
> @@ -19,7 +19,6 @@
>   #include "hw/qdev-properties.h"
>   #include "hw/usb/hcd-ehci.h"
>   #include "migration/vmstate.h"
> -#include "qemu/module.h"
>   
>   static const VMStateDescription vmstate_ehci_sysbus = {
>       .name        = "ehci-sysbus",
> @@ -97,17 +96,6 @@ static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_type_info = {
> -    .name          = TYPE_SYS_BUS_EHCI,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(EHCISysBusState),
> -    .instance_init = ehci_sysbus_init,
> -    .instance_finalize = ehci_sysbus_finalize,
> -    .abstract      = true,
> -    .class_init    = ehci_sysbus_class_init,
> -    .class_size    = sizeof(SysBusEHCIClass),
> -};
> -
>   static void ehci_platform_class_init(ObjectClass *oc, void *data)
>   {
>       SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> @@ -118,12 +106,6 @@ static void ehci_platform_class_init(ObjectClass *oc, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_platform_type_info = {
> -    .name          = TYPE_PLATFORM_EHCI,
> -    .parent        = TYPE_SYS_BUS_EHCI,
> -    .class_init    = ehci_platform_class_init,
> -};
> -
>   static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
>   {
>       SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> @@ -134,12 +116,6 @@ static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_exynos4210_type_info = {
> -    .name          = TYPE_EXYNOS4210_EHCI,
> -    .parent        = TYPE_SYS_BUS_EHCI,
> -    .class_init    = ehci_exynos4210_class_init,
> -};
> -
>   static void ehci_aw_h3_class_init(ObjectClass *oc, void *data)
>   {
>       SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> @@ -150,12 +126,6 @@ static void ehci_aw_h3_class_init(ObjectClass *oc, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_aw_h3_type_info = {
> -    .name          = TYPE_AW_H3_EHCI,
> -    .parent        = TYPE_SYS_BUS_EHCI,
> -    .class_init    = ehci_aw_h3_class_init,
> -};
> -
>   static void ehci_npcm7xx_class_init(ObjectClass *oc, void *data)
>   {
>       SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> @@ -168,12 +138,6 @@ static void ehci_npcm7xx_class_init(ObjectClass *oc, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_npcm7xx_type_info = {
> -    .name          = TYPE_NPCM7XX_EHCI,
> -    .parent        = TYPE_SYS_BUS_EHCI,
> -    .class_init    = ehci_npcm7xx_class_init,
> -};
> -
>   static void ehci_tegra2_class_init(ObjectClass *oc, void *data)
>   {
>       SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> @@ -184,12 +148,6 @@ static void ehci_tegra2_class_init(ObjectClass *oc, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_tegra2_type_info = {
> -    .name          = TYPE_TEGRA2_EHCI,
> -    .parent        = TYPE_SYS_BUS_EHCI,
> -    .class_init    = ehci_tegra2_class_init,
> -};
> -
>   static void ehci_ppc4xx_init(Object *o)
>   {
>       EHCISysBusState *s = SYS_BUS_EHCI(o);
> @@ -207,13 +165,6 @@ static void ehci_ppc4xx_class_init(ObjectClass *oc, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_ppc4xx_type_info = {
> -    .name          = TYPE_PPC4xx_EHCI,
> -    .parent        = TYPE_SYS_BUS_EHCI,
> -    .class_init    = ehci_ppc4xx_class_init,
> -    .instance_init = ehci_ppc4xx_init,
> -};
> -
>   /*
>    * Faraday FUSBH200 USB 2.0 EHCI
>    */
> @@ -282,24 +233,55 @@ static void fusbh200_ehci_class_init(ObjectClass *oc, void *data)
>       set_bit(DEVICE_CATEGORY_USB, dc->categories);
>   }
>   
> -static const TypeInfo ehci_fusbh200_type_info = {
> -    .name          = TYPE_FUSBH200_EHCI,
> -    .parent        = TYPE_SYS_BUS_EHCI,
> -    .instance_size = sizeof(FUSBH200EHCIState),
> -    .instance_init = fusbh200_ehci_init,
> -    .class_init    = fusbh200_ehci_class_init,
> +static const TypeInfo types[] = {
> +    {
> +        .name          = TYPE_SYS_BUS_EHCI,
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(EHCISysBusState),
> +        .instance_init = ehci_sysbus_init,
> +        .instance_finalize = ehci_sysbus_finalize,
> +        .abstract      = true,
> +        .class_init    = ehci_sysbus_class_init,
> +        .class_size    = sizeof(SysBusEHCIClass),
> +    },
> +    {
> +        .name          = TYPE_PLATFORM_EHCI,
> +        .parent        = TYPE_SYS_BUS_EHCI,
> +        .class_init    = ehci_platform_class_init,
> +    },
> +    {
> +        .name          = TYPE_EXYNOS4210_EHCI,
> +        .parent        = TYPE_SYS_BUS_EHCI,
> +        .class_init    = ehci_exynos4210_class_init,
> +    },
> +    {
> +        .name          = TYPE_AW_H3_EHCI,
> +        .parent        = TYPE_SYS_BUS_EHCI,
> +        .class_init    = ehci_aw_h3_class_init,
> +    },
> +    {
> +        .name          = TYPE_NPCM7XX_EHCI,
> +        .parent        = TYPE_SYS_BUS_EHCI,
> +        .class_init    = ehci_npcm7xx_class_init,
> +    },
> +    {
> +        .name          = TYPE_TEGRA2_EHCI,
> +        .parent        = TYPE_SYS_BUS_EHCI,
> +        .class_init    = ehci_tegra2_class_init,
> +    },
> +    {
> +        .name          = TYPE_PPC4xx_EHCI,
> +        .parent        = TYPE_SYS_BUS_EHCI,
> +        .class_init    = ehci_ppc4xx_class_init,
> +        .instance_init = ehci_ppc4xx_init,
> +    },
> +    {
> +        .name          = TYPE_FUSBH200_EHCI,
> +        .parent        = TYPE_SYS_BUS_EHCI,
> +        .instance_size = sizeof(FUSBH200EHCIState),
> +        .instance_init = fusbh200_ehci_init,
> +        .class_init    = fusbh200_ehci_class_init,
> +    },
>   };
>   
> -static void ehci_sysbus_register_types(void)
> -{
> -    type_register_static(&ehci_type_info);
> -    type_register_static(&ehci_platform_type_info);
> -    type_register_static(&ehci_exynos4210_type_info);
> -    type_register_static(&ehci_aw_h3_type_info);
> -    type_register_static(&ehci_npcm7xx_type_info);
> -    type_register_static(&ehci_tegra2_type_info);
> -    type_register_static(&ehci_ppc4xx_type_info);
> -    type_register_static(&ehci_fusbh200_type_info);
> -}
> -
> -type_init(ehci_sysbus_register_types)
> +DEFINE_TYPES(types)

[PATCH 23/23] hw/vfio/platform: Let vfio_start_eventfd_injection() take VFIOPlatformDevice pointer

[Bernhard Beschow]

Avoids one downcast, making the code more type-safe.

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

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index a85c199c76..77bbfbf62c 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -318,13 +318,12 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
 /**
  * vfio_start_eventfd_injection - starts the virtual IRQ injection using
  * user-side handled eventfds
- * @sbdev: the sysbus device handle
+ * @vdev: the VFIO platform device handle
  * @irq: the qemu irq handle
  */
 
-static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
+static void vfio_start_eventfd_injection(VFIOPlatformDevice *vdev, qemu_irq irq)
 {
-    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
     VFIOINTp *intp;
 
     QLIST_FOREACH(intp, &vdev->intp_list, next) {
@@ -417,7 +416,7 @@ fail_vfio:
     kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
     abort();
 fail_irqfd:
-    vfio_start_eventfd_injection(sbdev, irq);
+    vfio_start_eventfd_injection(vdev, irq);
     return;
 }
 
-- 
2.46.1

Re: [PATCH 23/23] hw/vfio/platform: Let vfio_start_eventfd_injection() take VFIOPlatformDevice pointer

[Cédric Le Goater]

On 9/23/24 11:30, Bernhard Beschow wrote:
> Avoids one downcast, making the code more type-safe.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

That's a bit unrelated to your e500 series. Anyhow,

Reviewed-by: Cédric Le Goater <clg@redhat.com>


Thanks,

C.


> ---
>   hw/vfio/platform.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index a85c199c76..77bbfbf62c 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -318,13 +318,12 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
>   /**
>    * vfio_start_eventfd_injection - starts the virtual IRQ injection using
>    * user-side handled eventfds
> - * @sbdev: the sysbus device handle
> + * @vdev: the VFIO platform device handle
>    * @irq: the qemu irq handle
>    */
>   
> -static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
> +static void vfio_start_eventfd_injection(VFIOPlatformDevice *vdev, qemu_irq irq)
>   {
> -    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>       VFIOINTp *intp;
>   
>       QLIST_FOREACH(intp, &vdev->intp_list, next) {
> @@ -417,7 +416,7 @@ fail_vfio:
>       kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
>       abort();
>   fail_irqfd:
> -    vfio_start_eventfd_injection(sbdev, irq);
> +    vfio_start_eventfd_injection(vdev, irq);
>       return;
>   }
>   

Re: [PATCH 00/23] E500 Cleanup

[Cédric Le Goater]

Hello Bernhard,

On 9/23/24 11:29, Bernhard Beschow wrote:
> This series is part of a bigger series exploring data-driven machine creation
> using device tree blobs on top of the e500 machines [1]. It contains patches to
> make this exploration easier which are also expected to provide value in
> themselves.
> 
> The cleanup starts with the e500 machine class itself, then proceeds with
> machine-specific device models and concludes with more or less loosely related
> devices. Device cleanup mostly consists of using the DEFINE_TYPES() macro.

Since you recently took a look at the machine models, would you
be willing to take over maintenance of the e500 ? It shouldn't
be an enormous amount of work.

Thanks,

C.



> [1] https://github.com/shentok/qemu/tree/e500-fdt
> 
> Bernhard Beschow (23):
>    hw/ppc/e500: Do not leak struct boot_info
>    hw/ppc/e500: Reduce scope of env pointer
>    hw/ppc/e500: Prefer QOM cast
>    hw/ppc/e500: Remove unused "irqs" parameter
>    hw/ppc/e500: Add missing device tree properties to i2c controller node
>    hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal
>      resources
>    hw/ppc/e500: Extract ppce500_ccsr.c
>    hw/ppc/ppce500_ccsr: Log access to unimplemented registers
>    hw/ppc/mpc8544_guts: Populate POR PLL ratio status register
>    hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access
>    hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro
>    hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define
>    hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro
>    hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro
>    hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro
>    hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro
>    hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch
>    hw/sd/sdhci: Prefer DEFINE_TYPES() macro
>    hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro
>    hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro
>    hw/rtc/ds1338: Prefer DEFINE_TYPES() macro
>    hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro
>    hw/vfio/platform: Let vfio_start_eventfd_injection() take
>      VFIOPlatformDevice pointer
> 
>   MAINTAINERS              |   2 +-
>   hw/ppc/e500-ccsr.h       |   2 +
>   hw/ppc/e500.h            |   8 +++
>   hw/block/pflash_cfi01.c  |  21 +++----
>   hw/gpio/mpc8xxx.c        |  22 +++-----
>   hw/i2c/mpc_i2c.c         |  29 +++++-----
>   hw/i2c/smbus_eeprom.c    |  19 +++----
>   hw/net/fsl_etsec/etsec.c |  22 +++-----
>   hw/pci-host/ppce500.c    |  54 ++++++++----------
>   hw/ppc/e500.c            |  61 +++++---------------
>   hw/ppc/mpc8544_guts.c    |  32 +++++++----
>   hw/ppc/ppce500_ccsr.c    |  67 ++++++++++++++++++++++
>   hw/rtc/ds1338.c          |  20 +++----
>   hw/sd/sdhci.c            |  62 +++++++++-----------
>   hw/usb/hcd-ehci-sysbus.c | 118 +++++++++++++++++----------------------
>   hw/vfio/platform.c       |   7 +--
>   hw/i2c/trace-events      |   5 ++
>   hw/intc/Kconfig          |   4 ++
>   hw/intc/meson.build      |   3 +-
>   hw/ppc/meson.build       |   1 +
>   hw/ppc/trace-events      |   3 +
>   21 files changed, 285 insertions(+), 277 deletions(-)
>   create mode 100644 hw/ppc/ppce500_ccsr.c
> 

Re: [PATCH 00/23] E500 Cleanup

[Bernhard Beschow]

Am 23. September 2024 20:23:54 UTC schrieb "Cédric Le Goater" <clg@redhat.com>:
>Hello Bernhard,

Hi Cédric,

>
>On 9/23/24 11:29, Bernhard Beschow wrote:
>> This series is part of a bigger series exploring data-driven machine creation
>> using device tree blobs on top of the e500 machines [1]. It contains patches to
>> make this exploration easier which are also expected to provide value in
>> themselves.
>> 
>> The cleanup starts with the e500 machine class itself, then proceeds with
>> machine-specific device models and concludes with more or less loosely related
>> devices. Device cleanup mostly consists of using the DEFINE_TYPES() macro.
>
>Since you recently took a look at the machine models, would you
>be willing to take over maintenance of the e500 ? It shouldn't
>be an enormous amount of work.

Are you referring to the machine and related devices or the CPU? I'm somewhat familiar with the P102x and could take over but the CPU would be a different beast.

Best regards,
Bernhard

>
>Thanks,
>
>C.
>
>
>
>> [1] https://github.com/shentok/qemu/tree/e500-fdt
>> 
>> Bernhard Beschow (23):
>>    hw/ppc/e500: Do not leak struct boot_info
>>    hw/ppc/e500: Reduce scope of env pointer
>>    hw/ppc/e500: Prefer QOM cast
>>    hw/ppc/e500: Remove unused "irqs" parameter
>>    hw/ppc/e500: Add missing device tree properties to i2c controller node
>>    hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal
>>      resources
>>    hw/ppc/e500: Extract ppce500_ccsr.c
>>    hw/ppc/ppce500_ccsr: Log access to unimplemented registers
>>    hw/ppc/mpc8544_guts: Populate POR PLL ratio status register
>>    hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access
>>    hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro
>>    hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define
>>    hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro
>>    hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro
>>    hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro
>>    hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro
>>    hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch
>>    hw/sd/sdhci: Prefer DEFINE_TYPES() macro
>>    hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro
>>    hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro
>>    hw/rtc/ds1338: Prefer DEFINE_TYPES() macro
>>    hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro
>>    hw/vfio/platform: Let vfio_start_eventfd_injection() take
>>      VFIOPlatformDevice pointer
>> 
>>   MAINTAINERS              |   2 +-
>>   hw/ppc/e500-ccsr.h       |   2 +
>>   hw/ppc/e500.h            |   8 +++
>>   hw/block/pflash_cfi01.c  |  21 +++----
>>   hw/gpio/mpc8xxx.c        |  22 +++-----
>>   hw/i2c/mpc_i2c.c         |  29 +++++-----
>>   hw/i2c/smbus_eeprom.c    |  19 +++----
>>   hw/net/fsl_etsec/etsec.c |  22 +++-----
>>   hw/pci-host/ppce500.c    |  54 ++++++++----------
>>   hw/ppc/e500.c            |  61 +++++---------------
>>   hw/ppc/mpc8544_guts.c    |  32 +++++++----
>>   hw/ppc/ppce500_ccsr.c    |  67 ++++++++++++++++++++++
>>   hw/rtc/ds1338.c          |  20 +++----
>>   hw/sd/sdhci.c            |  62 +++++++++-----------
>>   hw/usb/hcd-ehci-sysbus.c | 118 +++++++++++++++++----------------------
>>   hw/vfio/platform.c       |   7 +--
>>   hw/i2c/trace-events      |   5 ++
>>   hw/intc/Kconfig          |   4 ++
>>   hw/intc/meson.build      |   3 +-
>>   hw/ppc/meson.build       |   1 +
>>   hw/ppc/trace-events      |   3 +
>>   21 files changed, 285 insertions(+), 277 deletions(-)
>>   create mode 100644 hw/ppc/ppce500_ccsr.c
>> 
>

Re: [PATCH 00/23] E500 Cleanup

[Cédric Le Goater]

On 9/23/24 23:25, Bernhard Beschow wrote:
> 
> 
> 
> Am 23. September 2024 20:23:54 UTC schrieb "Cédric Le Goater" <clg@redhat.com>:
>> Hello Bernhard,
> 
> Hi Cédric,
> 
>>
>> On 9/23/24 11:29, Bernhard Beschow wrote:
>>> This series is part of a bigger series exploring data-driven machine creation
>>> using device tree blobs on top of the e500 machines [1]. It contains patches to
>>> make this exploration easier which are also expected to provide value in
>>> themselves.
>>>
>>> The cleanup starts with the e500 machine class itself, then proceeds with
>>> machine-specific device models and concludes with more or less loosely related
>>> devices. Device cleanup mostly consists of using the DEFINE_TYPES() macro.
>>
>> Since you recently took a look at the machine models, would you
>> be willing to take over maintenance of the e500 ? It shouldn't
>> be an enormous amount of work.
> 
> Are you referring to the machine and related devices or the CPU? I'm somewhat familiar with the P102x and could take over but the CPU would be a different beast.

Please take a look at the MAINTAINERS file. You will see it is not
that large and the CPU target models are not part of it.


Thanks,

C.

  


> 
> Best regards,
> Bernhard
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>> [1] https://github.com/shentok/qemu/tree/e500-fdt
>>>
>>> Bernhard Beschow (23):
>>>     hw/ppc/e500: Do not leak struct boot_info
>>>     hw/ppc/e500: Reduce scope of env pointer
>>>     hw/ppc/e500: Prefer QOM cast
>>>     hw/ppc/e500: Remove unused "irqs" parameter
>>>     hw/ppc/e500: Add missing device tree properties to i2c controller node
>>>     hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal
>>>       resources
>>>     hw/ppc/e500: Extract ppce500_ccsr.c
>>>     hw/ppc/ppce500_ccsr: Log access to unimplemented registers
>>>     hw/ppc/mpc8544_guts: Populate POR PLL ratio status register
>>>     hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access
>>>     hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro
>>>     hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define
>>>     hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro
>>>     hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro
>>>     hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro
>>>     hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro
>>>     hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch
>>>     hw/sd/sdhci: Prefer DEFINE_TYPES() macro
>>>     hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro
>>>     hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro
>>>     hw/rtc/ds1338: Prefer DEFINE_TYPES() macro
>>>     hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro
>>>     hw/vfio/platform: Let vfio_start_eventfd_injection() take
>>>       VFIOPlatformDevice pointer
>>>
>>>    MAINTAINERS              |   2 +-
>>>    hw/ppc/e500-ccsr.h       |   2 +
>>>    hw/ppc/e500.h            |   8 +++
>>>    hw/block/pflash_cfi01.c  |  21 +++----
>>>    hw/gpio/mpc8xxx.c        |  22 +++-----
>>>    hw/i2c/mpc_i2c.c         |  29 +++++-----
>>>    hw/i2c/smbus_eeprom.c    |  19 +++----
>>>    hw/net/fsl_etsec/etsec.c |  22 +++-----
>>>    hw/pci-host/ppce500.c    |  54 ++++++++----------
>>>    hw/ppc/e500.c            |  61 +++++---------------
>>>    hw/ppc/mpc8544_guts.c    |  32 +++++++----
>>>    hw/ppc/ppce500_ccsr.c    |  67 ++++++++++++++++++++++
>>>    hw/rtc/ds1338.c          |  20 +++----
>>>    hw/sd/sdhci.c            |  62 +++++++++-----------
>>>    hw/usb/hcd-ehci-sysbus.c | 118 +++++++++++++++++----------------------
>>>    hw/vfio/platform.c       |   7 +--
>>>    hw/i2c/trace-events      |   5 ++
>>>    hw/intc/Kconfig          |   4 ++
>>>    hw/intc/meson.build      |   3 +-
>>>    hw/ppc/meson.build       |   1 +
>>>    hw/ppc/trace-events      |   3 +
>>>    21 files changed, 285 insertions(+), 277 deletions(-)
>>>    create mode 100644 hw/ppc/ppce500_ccsr.c
>>>
>>
> 

Re: [PATCH 00/23] E500 Cleanup

[Bernhard Beschow]

Am 24. September 2024 08:33:26 UTC schrieb "Cédric Le Goater" <clg@redhat.com>:
>On 9/23/24 23:25, Bernhard Beschow wrote:
>> 
>> 
>> 
>> Am 23. September 2024 20:23:54 UTC schrieb "Cédric Le Goater" <clg@redhat.com>:
>>> Hello Bernhard,
>> 
>> Hi Cédric,
>> 
>>> 
>>> On 9/23/24 11:29, Bernhard Beschow wrote:
>>>> This series is part of a bigger series exploring data-driven machine creation
>>>> using device tree blobs on top of the e500 machines [1]. It contains patches to
>>>> make this exploration easier which are also expected to provide value in
>>>> themselves.
>>>> 
>>>> The cleanup starts with the e500 machine class itself, then proceeds with
>>>> machine-specific device models and concludes with more or less loosely related
>>>> devices. Device cleanup mostly consists of using the DEFINE_TYPES() macro.
>>> 
>>> Since you recently took a look at the machine models, would you
>>> be willing to take over maintenance of the e500 ? It shouldn't
>>> be an enormous amount of work.
>> 
>> Are you referring to the machine and related devices or the CPU? I'm somewhat familiar with the P102x and could take over but the CPU would be a different beast.
>
>Please take a look at the MAINTAINERS file. You will see it is not
>that large and the CPU target models are not part of it.

Patch sent: https://lore.kernel.org/qemu-devel/20240926075948.2343-1-shentey@gmail.com/

Best regards,
Bernhard

>
>
>Thanks,
>
>C.
>
> 
>
>> 
>> Best regards,
>> Bernhard
>> 
>>> 
>>> Thanks,
>>> 
>>> C.
>>> 
>>> 
>>> 
>>>> [1] https://github.com/shentok/qemu/tree/e500-fdt
>>>> 
>>>> Bernhard Beschow (23):
>>>>     hw/ppc/e500: Do not leak struct boot_info
>>>>     hw/ppc/e500: Reduce scope of env pointer
>>>>     hw/ppc/e500: Prefer QOM cast
>>>>     hw/ppc/e500: Remove unused "irqs" parameter
>>>>     hw/ppc/e500: Add missing device tree properties to i2c controller node
>>>>     hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal
>>>>       resources
>>>>     hw/ppc/e500: Extract ppce500_ccsr.c
>>>>     hw/ppc/ppce500_ccsr: Log access to unimplemented registers
>>>>     hw/ppc/mpc8544_guts: Populate POR PLL ratio status register
>>>>     hw/i2c/mpc_i2c: Convert DPRINTF to trace events for register access
>>>>     hw/i2c/mpc_i2c: Prefer DEFINE_TYPES() macro
>>>>     hw/pci-host/ppce500: Reuse TYPE_PPC_E500_PCI_BRIDGE define
>>>>     hw/pci-host/ppce500: Prefer DEFINE_TYPES() macro
>>>>     hw/gpio/mpc8xxx: Prefer DEFINE_TYPES() macro
>>>>     hw/ppc/mpc8544_guts: Prefer DEFINE_TYPES() macro
>>>>     hw/net/fsl_etsec/etsec: Prefer DEFINE_TYPES() macro
>>>>     hw/intc: Guard openpic_kvm.c by dedicated OPENPIC_KVM Kconfig switch
>>>>     hw/sd/sdhci: Prefer DEFINE_TYPES() macro
>>>>     hw/block/pflash_cfi01: Prefer DEFINE_TYPES() macro
>>>>     hw/i2c/smbus_eeprom: Prefer DEFINE_TYPES() macro
>>>>     hw/rtc/ds1338: Prefer DEFINE_TYPES() macro
>>>>     hw/usb/hcd-ehci-sysbus: Prefer DEFINE_TYPES() macro
>>>>     hw/vfio/platform: Let vfio_start_eventfd_injection() take
>>>>       VFIOPlatformDevice pointer
>>>> 
>>>>    MAINTAINERS              |   2 +-
>>>>    hw/ppc/e500-ccsr.h       |   2 +
>>>>    hw/ppc/e500.h            |   8 +++
>>>>    hw/block/pflash_cfi01.c  |  21 +++----
>>>>    hw/gpio/mpc8xxx.c        |  22 +++-----
>>>>    hw/i2c/mpc_i2c.c         |  29 +++++-----
>>>>    hw/i2c/smbus_eeprom.c    |  19 +++----
>>>>    hw/net/fsl_etsec/etsec.c |  22 +++-----
>>>>    hw/pci-host/ppce500.c    |  54 ++++++++----------
>>>>    hw/ppc/e500.c            |  61 +++++---------------
>>>>    hw/ppc/mpc8544_guts.c    |  32 +++++++----
>>>>    hw/ppc/ppce500_ccsr.c    |  67 ++++++++++++++++++++++
>>>>    hw/rtc/ds1338.c          |  20 +++----
>>>>    hw/sd/sdhci.c            |  62 +++++++++-----------
>>>>    hw/usb/hcd-ehci-sysbus.c | 118 +++++++++++++++++----------------------
>>>>    hw/vfio/platform.c       |   7 +--
>>>>    hw/i2c/trace-events      |   5 ++
>>>>    hw/intc/Kconfig          |   4 ++
>>>>    hw/intc/meson.build      |   3 +-
>>>>    hw/ppc/meson.build       |   1 +
>>>>    hw/ppc/trace-events      |   3 +
>>>>    21 files changed, 285 insertions(+), 277 deletions(-)
>>>>    create mode 100644 hw/ppc/ppce500_ccsr.c
>>>> 
>>> 
>> 
>