[PATCH 27/36] next-cube: QOMify NeXTRTC

Mark Cave-Ayland posted 36 patches 1 month ago
[PATCH 27/36] next-cube: QOMify NeXTRTC
Posted by Mark Cave-Ayland 1 month ago
This is to allow the RTC functionality to be maintained within its own separate
device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index e4d0083eb0..6b574d39cf 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -42,7 +42,13 @@
 #define RAM_SIZE    0x4000000
 #define ROM_FILE    "Rev_2.5_v66.bin"
 
-typedef struct NeXTRTC {
+
+#define TYPE_NEXT_RTC "next-rtc"
+OBJECT_DECLARE_SIMPLE_TYPE(NeXTRTC, NEXT_RTC)
+
+struct NeXTRTC {
+    SysBusDevice parent_obj;
+
     int8_t phase;
     uint8_t ram[32];
     uint8_t command;
@@ -50,7 +56,7 @@ typedef struct NeXTRTC {
     uint8_t status;
     uint8_t control;
     uint8_t retval;
-} NeXTRTC;
+};
 
 #define TYPE_NEXT_SCSI "next-scsi"
 OBJECT_DECLARE_SIMPLE_TYPE(NeXTSCSI, NEXT_SCSI)
@@ -1012,6 +1018,37 @@ static const MemoryRegionOps next_dummy_en_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+static const VMStateDescription next_rtc_vmstate = {
+    .name = "next-rtc",
+    .version_id = 3,
+    .minimum_version_id = 3,
+    .fields = (const VMStateField[]) {
+        VMSTATE_INT8(phase, NeXTRTC),
+        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
+        VMSTATE_UINT8(command, NeXTRTC),
+        VMSTATE_UINT8(value, NeXTRTC),
+        VMSTATE_UINT8(status, NeXTRTC),
+        VMSTATE_UINT8(control, NeXTRTC),
+        VMSTATE_UINT8(retval, NeXTRTC),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void next_rtc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "NeXT RTC";
+    dc->vmsd = &next_rtc_vmstate;
+}
+
+static const TypeInfo next_rtc_info = {
+    .name = TYPE_NEXT_RTC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(NeXTRTC),
+    .class_init = next_rtc_class_init,
+};
+
 static void next_pc_rtc_data_in_irq(void *opaque, int n, int level)
 {
     NeXTPC *s = NEXT_PC(opaque);
@@ -1078,6 +1115,12 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     }
     sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(dev, NEXT_SCC_I));
     sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(dev, NEXT_SCC_DMA_I));
+
+    /* RTC */
+    d = DEVICE(object_resolve_path_component(OBJECT(dev), "rtc"));
+    if (!sysbus_realize(SYS_BUS_DEVICE(d), errp)) {
+        return;
+    }
 }
 
 static void next_pc_init(Object *obj)
@@ -1111,6 +1154,8 @@ static void next_pc_init(Object *obj)
                           "next.timer", 4);
     sysbus_init_mmio(sbd, &s->timer_mem);
 
+    object_initialize_child(obj, "rtc", &s->rtc, TYPE_NEXT_RTC);
+
     s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
     qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
                             "pc-rtc-data-in", 1);
@@ -1129,22 +1174,6 @@ static Property next_pc_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static const VMStateDescription next_rtc_vmstate = {
-    .name = "next-rtc",
-    .version_id = 2,
-    .minimum_version_id = 2,
-    .fields = (const VMStateField[]) {
-        VMSTATE_INT8(phase, NeXTRTC),
-        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
-        VMSTATE_UINT8(command, NeXTRTC),
-        VMSTATE_UINT8(value, NeXTRTC),
-        VMSTATE_UINT8(status, NeXTRTC),
-        VMSTATE_UINT8(control, NeXTRTC),
-        VMSTATE_UINT8(retval, NeXTRTC),
-        VMSTATE_END_OF_LIST()
-    },
-};
-
 static const VMStateDescription next_pc_vmstate = {
     .name = "next-pc",
     .version_id = 3,
@@ -1297,6 +1326,7 @@ static void next_register_type(void)
     type_register_static(&next_typeinfo);
     type_register_static(&next_pc_info);
     type_register_static(&next_scsi_info);
+    type_register_static(&next_rtc_info);
 }
 
 type_init(next_register_type)
-- 
2.39.5
Re: [PATCH 27/36] next-cube: QOMify NeXTRTC
Posted by Thomas Huth 2 weeks ago
Am Wed, 23 Oct 2024 09:58:43 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> This is to allow the RTC functionality to be maintained within its own separate
> device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index e4d0083eb0..6b574d39cf 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -42,7 +42,13 @@
>  #define RAM_SIZE    0x4000000
>  #define ROM_FILE    "Rev_2.5_v66.bin"
>  
> -typedef struct NeXTRTC {
> +
> +#define TYPE_NEXT_RTC "next-rtc"
> +OBJECT_DECLARE_SIMPLE_TYPE(NeXTRTC, NEXT_RTC)
> +
> +struct NeXTRTC {
> +    SysBusDevice parent_obj;
> +
>      int8_t phase;
>      uint8_t ram[32];
>      uint8_t command;
> @@ -50,7 +56,7 @@ typedef struct NeXTRTC {
>      uint8_t status;
>      uint8_t control;
>      uint8_t retval;
> -} NeXTRTC;
> +};
>  
>  #define TYPE_NEXT_SCSI "next-scsi"
>  OBJECT_DECLARE_SIMPLE_TYPE(NeXTSCSI, NEXT_SCSI)
> @@ -1012,6 +1018,37 @@ static const MemoryRegionOps next_dummy_en_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> +static const VMStateDescription next_rtc_vmstate = {
> +    .name = "next-rtc",
> +    .version_id = 3,
> +    .minimum_version_id = 3,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_INT8(phase, NeXTRTC),
> +        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
> +        VMSTATE_UINT8(command, NeXTRTC),
> +        VMSTATE_UINT8(value, NeXTRTC),
> +        VMSTATE_UINT8(status, NeXTRTC),
> +        VMSTATE_UINT8(control, NeXTRTC),
> +        VMSTATE_UINT8(retval, NeXTRTC),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static void next_rtc_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc = "NeXT RTC";
> +    dc->vmsd = &next_rtc_vmstate;
> +}
> +
> +static const TypeInfo next_rtc_info = {
> +    .name = TYPE_NEXT_RTC,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(NeXTRTC),
> +    .class_init = next_rtc_class_init,
> +};
> +
>  static void next_pc_rtc_data_in_irq(void *opaque, int n, int level)
>  {
>      NeXTPC *s = NEXT_PC(opaque);
> @@ -1078,6 +1115,12 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
>      }
>      sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(dev, NEXT_SCC_I));
>      sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(dev, NEXT_SCC_DMA_I));
> +
> +    /* RTC */
> +    d = DEVICE(object_resolve_path_component(OBJECT(dev), "rtc"));
> +    if (!sysbus_realize(SYS_BUS_DEVICE(d), errp)) {
> +        return;
> +    }
>  }
>  
>  static void next_pc_init(Object *obj)
> @@ -1111,6 +1154,8 @@ static void next_pc_init(Object *obj)
>                            "next.timer", 4);
>      sysbus_init_mmio(sbd, &s->timer_mem);
>  
> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_NEXT_RTC);
> +
>      s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
>      qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
>                              "pc-rtc-data-in", 1);
> @@ -1129,22 +1174,6 @@ static Property next_pc_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -static const VMStateDescription next_rtc_vmstate = {
> -    .name = "next-rtc",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> -    .fields = (const VMStateField[]) {
> -        VMSTATE_INT8(phase, NeXTRTC),
> -        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
> -        VMSTATE_UINT8(command, NeXTRTC),
> -        VMSTATE_UINT8(value, NeXTRTC),
> -        VMSTATE_UINT8(status, NeXTRTC),
> -        VMSTATE_UINT8(control, NeXTRTC),
> -        VMSTATE_UINT8(retval, NeXTRTC),
> -        VMSTATE_END_OF_LIST()
> -    },
> -};
> -
>  static const VMStateDescription next_pc_vmstate = {
>      .name = "next-pc",
>      .version_id = 3,
> @@ -1297,6 +1326,7 @@ static void next_register_type(void)
>      type_register_static(&next_typeinfo);
>      type_register_static(&next_pc_info);
>      type_register_static(&next_scsi_info);
> +    type_register_static(&next_rtc_info);
>  }
>  
>  type_init(next_register_type)

Shouldn't the next_rtc_vmstate get removed from next_pc_vmstate now?

Also, should we finally move the RTC code to a separate file?

 Thomas
Re: [PATCH 27/36] next-cube: QOMify NeXTRTC
Posted by Mark Cave-Ayland 1 week, 4 days ago
On 09/11/2024 08:14, Thomas Huth wrote:

> Am Wed, 23 Oct 2024 09:58:43 +0100
> schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> This is to allow the RTC functionality to be maintained within its own separate
>> device.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 48 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index e4d0083eb0..6b574d39cf 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -42,7 +42,13 @@
>>   #define RAM_SIZE    0x4000000
>>   #define ROM_FILE    "Rev_2.5_v66.bin"
>>   
>> -typedef struct NeXTRTC {
>> +
>> +#define TYPE_NEXT_RTC "next-rtc"
>> +OBJECT_DECLARE_SIMPLE_TYPE(NeXTRTC, NEXT_RTC)
>> +
>> +struct NeXTRTC {
>> +    SysBusDevice parent_obj;
>> +
>>       int8_t phase;
>>       uint8_t ram[32];
>>       uint8_t command;
>> @@ -50,7 +56,7 @@ typedef struct NeXTRTC {
>>       uint8_t status;
>>       uint8_t control;
>>       uint8_t retval;
>> -} NeXTRTC;
>> +};
>>   
>>   #define TYPE_NEXT_SCSI "next-scsi"
>>   OBJECT_DECLARE_SIMPLE_TYPE(NeXTSCSI, NEXT_SCSI)
>> @@ -1012,6 +1018,37 @@ static const MemoryRegionOps next_dummy_en_ops = {
>>       .endianness = DEVICE_BIG_ENDIAN,
>>   };
>>   
>> +static const VMStateDescription next_rtc_vmstate = {
>> +    .name = "next-rtc",
>> +    .version_id = 3,
>> +    .minimum_version_id = 3,
>> +    .fields = (const VMStateField[]) {
>> +        VMSTATE_INT8(phase, NeXTRTC),
>> +        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
>> +        VMSTATE_UINT8(command, NeXTRTC),
>> +        VMSTATE_UINT8(value, NeXTRTC),
>> +        VMSTATE_UINT8(status, NeXTRTC),
>> +        VMSTATE_UINT8(control, NeXTRTC),
>> +        VMSTATE_UINT8(retval, NeXTRTC),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>> +static void next_rtc_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->desc = "NeXT RTC";
>> +    dc->vmsd = &next_rtc_vmstate;
>> +}
>> +
>> +static const TypeInfo next_rtc_info = {
>> +    .name = TYPE_NEXT_RTC,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(NeXTRTC),
>> +    .class_init = next_rtc_class_init,
>> +};
>> +
>>   static void next_pc_rtc_data_in_irq(void *opaque, int n, int level)
>>   {
>>       NeXTPC *s = NEXT_PC(opaque);
>> @@ -1078,6 +1115,12 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
>>       }
>>       sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(dev, NEXT_SCC_I));
>>       sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(dev, NEXT_SCC_DMA_I));
>> +
>> +    /* RTC */
>> +    d = DEVICE(object_resolve_path_component(OBJECT(dev), "rtc"));
>> +    if (!sysbus_realize(SYS_BUS_DEVICE(d), errp)) {
>> +        return;
>> +    }
>>   }
>>   
>>   static void next_pc_init(Object *obj)
>> @@ -1111,6 +1154,8 @@ static void next_pc_init(Object *obj)
>>                             "next.timer", 4);
>>       sysbus_init_mmio(sbd, &s->timer_mem);
>>   
>> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_NEXT_RTC);
>> +
>>       s->rtc_power_irq = qdev_get_gpio_in(DEVICE(obj), NEXT_PWR_I);
>>       qdev_init_gpio_in_named(DEVICE(obj), next_pc_rtc_data_in_irq,
>>                               "pc-rtc-data-in", 1);
>> @@ -1129,22 +1174,6 @@ static Property next_pc_properties[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> -static const VMStateDescription next_rtc_vmstate = {
>> -    .name = "next-rtc",
>> -    .version_id = 2,
>> -    .minimum_version_id = 2,
>> -    .fields = (const VMStateField[]) {
>> -        VMSTATE_INT8(phase, NeXTRTC),
>> -        VMSTATE_UINT8_ARRAY(ram, NeXTRTC, 32),
>> -        VMSTATE_UINT8(command, NeXTRTC),
>> -        VMSTATE_UINT8(value, NeXTRTC),
>> -        VMSTATE_UINT8(status, NeXTRTC),
>> -        VMSTATE_UINT8(control, NeXTRTC),
>> -        VMSTATE_UINT8(retval, NeXTRTC),
>> -        VMSTATE_END_OF_LIST()
>> -    },
>> -};
>> -
>>   static const VMStateDescription next_pc_vmstate = {
>>       .name = "next-pc",
>>       .version_id = 3,
>> @@ -1297,6 +1326,7 @@ static void next_register_type(void)
>>       type_register_static(&next_typeinfo);
>>       type_register_static(&next_pc_info);
>>       type_register_static(&next_scsi_info);
>> +    type_register_static(&next_rtc_info);
>>   }
>>   
>>   type_init(next_register_type)
> 
> Shouldn't the next_rtc_vmstate get removed from next_pc_vmstate now?

Indeed, yes it should. I'll fix that up for v2.

> Also, should we finally move the RTC code to a separate file?

I was thinking about whether it was worth splitting up next-cube.c whilst writing 
this series, but I didn't come up with a solution I was completely happy with. It is 
certainly something to reconsider in future though.


ATB,

Mark.
Re: [PATCH 27/36] next-cube: QOMify NeXTRTC
Posted by Philippe Mathieu-Daudé 1 month ago
Hi Mark,

On 23/10/24 05:58, Mark Cave-Ayland wrote:
> This is to allow the RTC functionality to be maintained within its own separate
> device.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>   1 file changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index e4d0083eb0..6b574d39cf 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -42,7 +42,13 @@
>   #define RAM_SIZE    0x4000000
>   #define ROM_FILE    "Rev_2.5_v66.bin"
>   
> -typedef struct NeXTRTC {
> +
> +#define TYPE_NEXT_RTC "next-rtc"
> +OBJECT_DECLARE_SIMPLE_TYPE(NeXTRTC, NEXT_RTC)
> +
> +struct NeXTRTC {
> +    SysBusDevice parent_obj;

Since it was not explicitly reset, maybe QDev parent is enough?

>       int8_t phase;
>       uint8_t ram[32];
>       uint8_t command;
> @@ -50,7 +56,7 @@ typedef struct NeXTRTC {
>       uint8_t status;
>       uint8_t control;
>       uint8_t retval;
> -} NeXTRTC;
> +};
Re: [PATCH 27/36] next-cube: QOMify NeXTRTC
Posted by Mark Cave-Ayland 1 month ago
On 24/10/2024 03:44, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 23/10/24 05:58, Mark Cave-Ayland wrote:
>> This is to allow the RTC functionality to be maintained within its own separate
>> device.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/next-cube.c | 66 ++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 48 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index e4d0083eb0..6b574d39cf 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -42,7 +42,13 @@
>>   #define RAM_SIZE    0x4000000
>>   #define ROM_FILE    "Rev_2.5_v66.bin"
>> -typedef struct NeXTRTC {
>> +
>> +#define TYPE_NEXT_RTC "next-rtc"
>> +OBJECT_DECLARE_SIMPLE_TYPE(NeXTRTC, NEXT_RTC)
>> +
>> +struct NeXTRTC {
>> +    SysBusDevice parent_obj;
> 
> Since it was not explicitly reset, maybe QDev parent is enough?

Hi Phil,

This is deliberate, since the next-pc device resets a couple of fields directly in 
the NeXTRTC struct in next_pc_reset_hold(), and these are moved to a separate 
next_rtc_reset_hold() in the next patch.

>>       int8_t phase;
>>       uint8_t ram[32];
>>       uint8_t command;
>> @@ -50,7 +56,7 @@ typedef struct NeXTRTC {
>>       uint8_t status;
>>       uint8_t control;
>>       uint8_t retval;
>> -} NeXTRTC;
>> +};


ATB,

Mark.