qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] m48t59: remove legacy init functions
@ 2020-10-16 18:27 Mark Cave-Ayland
  2020-10-16 18:27 ` [PATCH 1/5] m48t59-isa: remove legacy m48t59_init_isa() function Mark Cave-Ayland
                   ` (5 more replies)
  0 siblings, 6 replies; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-16 18:27 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, f4bug, hpoussin, qemu-ppc, atar4qemu, david

This patchset is inspired by Philippe's "hw/rtc/m48t59: Simplify m48t59_init()"
patchset at https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg04493.html
but goes further: rather than tidy-up the legacy init functions, convert the
callers to use qdev properties directly so they can simply be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (5):
  m48t59-isa: remove legacy m48t59_init_isa() function
  sun4m: use qdev properties instead of legacy m48t59_init() function
  sun4u: use qdev properties instead of legacy m48t59_init() function
  ppc405_boards: use qdev properties instead of legacy m48t59_init()
    function
  m48t59: remove legacy m48t59_init() function

 hw/ppc/ppc405_boards.c  | 10 +++++++++-
 hw/rtc/m48t59-isa.c     | 25 -------------------------
 hw/rtc/m48t59.c         | 35 -----------------------------------
 hw/sparc/sun4m.c        |  8 +++++++-
 hw/sparc64/sun4u.c      |  7 +++++--
 include/hw/rtc/m48t59.h |  6 ------
 6 files changed, 21 insertions(+), 70 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/5] m48t59-isa: remove legacy m48t59_init_isa() function
  2020-10-16 18:27 [PATCH 0/5] m48t59: remove legacy init functions Mark Cave-Ayland
@ 2020-10-16 18:27 ` Mark Cave-Ayland
  2020-10-17  9:39   ` Philippe Mathieu-Daudé
  2020-10-16 18:27 ` [PATCH 2/5] sun4m: use qdev properties instead of legacy m48t59_init() function Mark Cave-Ayland
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-16 18:27 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, f4bug, hpoussin, qemu-ppc, atar4qemu, david

This function is no longer used within the codebase.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/rtc/m48t59-isa.c     | 25 -------------------------
 include/hw/rtc/m48t59.h |  2 --
 2 files changed, 27 deletions(-)

diff --git a/hw/rtc/m48t59-isa.c b/hw/rtc/m48t59-isa.c
index cae315e488..dc21fb10a5 100644
--- a/hw/rtc/m48t59-isa.c
+++ b/hw/rtc/m48t59-isa.c
@@ -58,31 +58,6 @@ static M48txxInfo m48txx_isa_info[] = {
     }
 };
 
-Nvram *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size,
-                       int base_year, int model)
-{
-    ISADevice *isa_dev;
-    DeviceState *dev;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(m48txx_isa_info); i++) {
-        if (m48txx_isa_info[i].size != size ||
-            m48txx_isa_info[i].model != model) {
-            continue;
-        }
-
-        isa_dev = isa_new(m48txx_isa_info[i].bus_name);
-        dev = DEVICE(isa_dev);
-        qdev_prop_set_uint32(dev, "iobase", io_base);
-        qdev_prop_set_int32(dev, "base-year", base_year);
-        isa_realize_and_unref(isa_dev, bus, &error_fatal);
-        return NVRAM(dev);
-    }
-
-    assert(false);
-    return NULL;
-}
-
 static uint32_t m48txx_isa_read(Nvram *obj, uint32_t addr)
 {
     M48txxISAState *d = M48TXX_ISA(obj);
diff --git a/include/hw/rtc/m48t59.h b/include/hw/rtc/m48t59.h
index 04abedf3b2..9defe578d1 100644
--- a/include/hw/rtc/m48t59.h
+++ b/include/hw/rtc/m48t59.h
@@ -47,8 +47,6 @@ struct NvramClass {
     void (*toggle_lock)(Nvram *obj, int lock);
 };
 
-Nvram *m48t59_init_isa(ISABus *bus, uint32_t io_base, uint16_t size,
-                       int base_year, int type);
 Nvram *m48t59_init(qemu_irq IRQ, hwaddr mem_base,
                    uint32_t io_base, uint16_t size, int base_year,
                    int type);
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/5] sun4m: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 18:27 [PATCH 0/5] m48t59: remove legacy init functions Mark Cave-Ayland
  2020-10-16 18:27 ` [PATCH 1/5] m48t59-isa: remove legacy m48t59_init_isa() function Mark Cave-Ayland
@ 2020-10-16 18:27 ` Mark Cave-Ayland
  2020-10-17  9:42   ` Philippe Mathieu-Daudé
  2020-10-16 18:27 ` [PATCH 3/5] sun4u: " Mark Cave-Ayland
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-16 18:27 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, f4bug, hpoussin, qemu-ppc, atar4qemu, david

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc/sun4m.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 54a2b2f9ef..a9bb60f2b2 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -966,7 +966,13 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
         create_unimplemented_device("SUNW,sx", hwdef->sx_base, 0x2000);
     }
 
-    nvram = m48t59_init(slavio_irq[0], hwdef->nvram_base, 0, 0x2000, 1968, 8);
+    dev = qdev_new("sysbus-m48t08");
+    qdev_prop_set_int32(dev, "base-year", 1968);
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(s, &error_fatal);
+    sysbus_connect_irq(s, 0, slavio_irq[0]);
+    sysbus_mmio_map(s, 0, hwdef->nvram_base);
+    nvram = NVRAM(dev);
 
     slavio_timer_init_all(hwdef->counter_base, slavio_irq[19], slavio_cpu_irq, smp_cpus);
 
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/5] sun4u: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 18:27 [PATCH 0/5] m48t59: remove legacy init functions Mark Cave-Ayland
  2020-10-16 18:27 ` [PATCH 1/5] m48t59-isa: remove legacy m48t59_init_isa() function Mark Cave-Ayland
  2020-10-16 18:27 ` [PATCH 2/5] sun4m: use qdev properties instead of legacy m48t59_init() function Mark Cave-Ayland
@ 2020-10-16 18:27 ` Mark Cave-Ayland
  2020-10-17  9:43   ` Philippe Mathieu-Daudé
  2020-10-16 18:27 ` [PATCH 4/5] ppc405_boards: " Mark Cave-Ayland
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-16 18:27 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, f4bug, hpoussin, qemu-ppc, atar4qemu, david

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc64/sun4u.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index ad5ca2472a..05e659c8a4 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -671,10 +671,13 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     pci_ide_create_devs(pci_dev);
 
     /* Map NVRAM into I/O (ebus) space */
-    nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
-    s = SYS_BUS_DEVICE(nvram);
+    dev = qdev_new("sysbus-m48t59");
+    qdev_prop_set_int32(dev, "base-year", 1968);
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(s, &error_fatal);
     memory_region_add_subregion(pci_address_space_io(ebus), 0x2000,
                                 sysbus_mmio_get_region(s, 0));
+    nvram = NVRAM(dev);
  
     initrd_size = 0;
     initrd_addr = 0;
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 18:27 [PATCH 0/5] m48t59: remove legacy init functions Mark Cave-Ayland
                   ` (2 preceding siblings ...)
  2020-10-16 18:27 ` [PATCH 3/5] sun4u: " Mark Cave-Ayland
@ 2020-10-16 18:27 ` Mark Cave-Ayland
  2020-10-16 20:38   ` BALATON Zoltan via
  2020-10-16 18:27 ` [PATCH 5/5] m48t59: remove " Mark Cave-Ayland
  2020-10-16 20:21 ` [PATCH 0/5] m48t59: remove legacy init functions Hervé Poussineau
  5 siblings, 1 reply; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-16 18:27 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, f4bug, hpoussin, qemu-ppc, atar4qemu, david

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/ppc405_boards.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 6198ec1035..4687715b15 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -28,6 +28,8 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "hw/ppc/ppc.h"
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
 #include "ppc405.h"
 #include "hw/rtc/m48t59.h"
 #include "hw/block/flash.h"
@@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
     char *filename;
     ppc4xx_bd_info_t bd;
     CPUPPCState *env;
+    DeviceState *dev;
+    SysBusDevice *s;
     qemu_irq *pic;
     MemoryRegion *bios;
     MemoryRegion *sram = g_new(MemoryRegion, 1);
@@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
     /* Register FPGA */
     ref405ep_fpga_init(sysmem, 0xF0300000);
     /* Register NVRAM */
-    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
+    dev = qdev_new("sysbus-m48t08");
+    qdev_prop_set_int32(dev, "base-year", 1968);
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(s, &error_fatal);
+    sysbus_mmio_map(s, 0, 0xF0000000);
     /* Load kernel */
     linux_boot = (kernel_filename != NULL);
     if (linux_boot) {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 5/5] m48t59: remove legacy m48t59_init() function
  2020-10-16 18:27 [PATCH 0/5] m48t59: remove legacy init functions Mark Cave-Ayland
                   ` (3 preceding siblings ...)
  2020-10-16 18:27 ` [PATCH 4/5] ppc405_boards: " Mark Cave-Ayland
@ 2020-10-16 18:27 ` Mark Cave-Ayland
  2020-10-17  9:53   ` Philippe Mathieu-Daudé
  2020-10-16 20:21 ` [PATCH 0/5] m48t59: remove legacy init functions Hervé Poussineau
  5 siblings, 1 reply; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-16 18:27 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial, f4bug, hpoussin, qemu-ppc, atar4qemu, david

Now that all of the callers of this function have been switched to use qdev
properties, this legacy init function can now be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/rtc/m48t59.c         | 35 -----------------------------------
 include/hw/rtc/m48t59.h |  4 ----
 2 files changed, 39 deletions(-)

diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
index 6525206976..d54929e861 100644
--- a/hw/rtc/m48t59.c
+++ b/hw/rtc/m48t59.c
@@ -564,41 +564,6 @@ const MemoryRegionOps m48t59_io_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-/* Initialisation routine */
-Nvram *m48t59_init(qemu_irq IRQ, hwaddr mem_base,
-                   uint32_t io_base, uint16_t size, int base_year,
-                   int model)
-{
-    DeviceState *dev;
-    SysBusDevice *s;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(m48txx_sysbus_info); i++) {
-        if (m48txx_sysbus_info[i].size != size ||
-            m48txx_sysbus_info[i].model != model) {
-            continue;
-        }
-
-        dev = qdev_new(m48txx_sysbus_info[i].bus_name);
-        qdev_prop_set_int32(dev, "base-year", base_year);
-        s = SYS_BUS_DEVICE(dev);
-        sysbus_realize_and_unref(s, &error_fatal);
-        sysbus_connect_irq(s, 0, IRQ);
-        if (io_base != 0) {
-            memory_region_add_subregion(get_system_io(), io_base,
-                                        sysbus_mmio_get_region(s, 1));
-        }
-        if (mem_base != 0) {
-            sysbus_mmio_map(s, 0, mem_base);
-        }
-
-        return NVRAM(s);
-    }
-
-    assert(false);
-    return NULL;
-}
-
 void m48t59_realize_common(M48t59State *s, Error **errp)
 {
     s->buffer = g_malloc0(s->size);
diff --git a/include/hw/rtc/m48t59.h b/include/hw/rtc/m48t59.h
index 9defe578d1..d9b45eb161 100644
--- a/include/hw/rtc/m48t59.h
+++ b/include/hw/rtc/m48t59.h
@@ -47,8 +47,4 @@ struct NvramClass {
     void (*toggle_lock)(Nvram *obj, int lock);
 };
 
-Nvram *m48t59_init(qemu_irq IRQ, hwaddr mem_base,
-                   uint32_t io_base, uint16_t size, int base_year,
-                   int type);
-
 #endif /* HW_M48T59_H */
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 0/5] m48t59: remove legacy init functions
  2020-10-16 18:27 [PATCH 0/5] m48t59: remove legacy init functions Mark Cave-Ayland
                   ` (4 preceding siblings ...)
  2020-10-16 18:27 ` [PATCH 5/5] m48t59: remove " Mark Cave-Ayland
@ 2020-10-16 20:21 ` Hervé Poussineau
  5 siblings, 0 replies; 20+ messages in thread
From: Hervé Poussineau @ 2020-10-16 20:21 UTC (permalink / raw)
  To: Mark Cave-Ayland, qemu-devel, qemu-trivial, f4bug, qemu-ppc,
	atar4qemu, david

Le 16/10/2020 à 20:27, Mark Cave-Ayland a écrit :
> This patchset is inspired by Philippe's "hw/rtc/m48t59: Simplify m48t59_init()"
> patchset at https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg04493.html
> but goes further: rather than tidy-up the legacy init functions, convert the
> callers to use qdev properties directly so they can simply be removed.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (5):
>    m48t59-isa: remove legacy m48t59_init_isa() function
>    sun4m: use qdev properties instead of legacy m48t59_init() function
>    sun4u: use qdev properties instead of legacy m48t59_init() function
>    ppc405_boards: use qdev properties instead of legacy m48t59_init()
>      function
>    m48t59: remove legacy m48t59_init() function
> 
>   hw/ppc/ppc405_boards.c  | 10 +++++++++-
>   hw/rtc/m48t59-isa.c     | 25 -------------------------
>   hw/rtc/m48t59.c         | 35 -----------------------------------
>   hw/sparc/sun4m.c        |  8 +++++++-
>   hw/sparc64/sun4u.c      |  7 +++++--
>   include/hw/rtc/m48t59.h |  6 ------
>   6 files changed, 21 insertions(+), 70 deletions(-)
> 

Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 18:27 ` [PATCH 4/5] ppc405_boards: " Mark Cave-Ayland
@ 2020-10-16 20:38   ` BALATON Zoltan via
  2020-10-17  9:45     ` Philippe Mathieu-Daudé
  2020-10-17 14:41     ` Artyom Tarasenko
  0 siblings, 2 replies; 20+ messages in thread
From: BALATON Zoltan via @ 2020-10-16 20:38 UTC (permalink / raw)
  To: Mark Cave-Ayland
  Cc: qemu-trivial, qemu-devel, f4bug, qemu-ppc, hpoussin, atar4qemu, david

On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/ppc/ppc405_boards.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 6198ec1035..4687715b15 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -28,6 +28,8 @@
> #include "qemu-common.h"
> #include "cpu.h"
> #include "hw/ppc/ppc.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/sysbus.h"
> #include "ppc405.h"
> #include "hw/rtc/m48t59.h"
> #include "hw/block/flash.h"
> @@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
>     char *filename;
>     ppc4xx_bd_info_t bd;
>     CPUPPCState *env;
> +    DeviceState *dev;
> +    SysBusDevice *s;
>     qemu_irq *pic;
>     MemoryRegion *bios;
>     MemoryRegion *sram = g_new(MemoryRegion, 1);
> @@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
>     /* Register FPGA */
>     ref405ep_fpga_init(sysmem, 0xF0300000);
>     /* Register NVRAM */
> -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
> +    dev = qdev_new("sysbus-m48t08");
> +    qdev_prop_set_int32(dev, "base-year", 1968);

Is there anything that uses other than 1968 as base year? If not this 
could be the default in the device so you don't need these set prop calls 
here and in sun machines.

The only other place this device is used seems to be ppc/prep machine that 
uses the isa version but does not set a base year. Is that a bug? The 
original prep machine removed in b2ce76a0730 used 2000 but that's unlikely 
as well as these machines predate that. Anyway, the sysbus and isa 
versions are different so their default base-year could be set 
independently and then boards won't need to set this propery and may be 
could use qdev_create_simple instead or whatever that was renamed to.

Regards,
BALATON Zoltan

> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(s, &error_fatal);
> +    sysbus_mmio_map(s, 0, 0xF0000000);
>     /* Load kernel */
>     linux_boot = (kernel_filename != NULL);
>     if (linux_boot) {
>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/5] m48t59-isa: remove legacy m48t59_init_isa() function
  2020-10-16 18:27 ` [PATCH 1/5] m48t59-isa: remove legacy m48t59_init_isa() function Mark Cave-Ayland
@ 2020-10-17  9:39   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17  9:39 UTC (permalink / raw)
  To: Mark Cave-Ayland, qemu-devel, qemu-trivial, hpoussin, qemu-ppc,
	atar4qemu, david

On 10/16/20 8:27 PM, Mark Cave-Ayland wrote:
> This function is no longer used within the codebase.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/rtc/m48t59-isa.c     | 25 -------------------------
>   include/hw/rtc/m48t59.h |  2 --
>   2 files changed, 27 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/5] sun4m: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 18:27 ` [PATCH 2/5] sun4m: use qdev properties instead of legacy m48t59_init() function Mark Cave-Ayland
@ 2020-10-17  9:42   ` Philippe Mathieu-Daudé
  2020-10-17 11:07     ` Mark Cave-Ayland
  0 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17  9:42 UTC (permalink / raw)
  To: Mark Cave-Ayland, qemu-devel, qemu-trivial, hpoussin, qemu-ppc,
	atar4qemu, david



On 10/16/20 8:27 PM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/sparc/sun4m.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 54a2b2f9ef..a9bb60f2b2 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -966,7 +966,13 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>           create_unimplemented_device("SUNW,sx", hwdef->sx_base, 0x2000);
>       }
>   
> -    nvram = m48t59_init(slavio_irq[0], hwdef->nvram_base, 0, 0x2000, 1968, 8);
> +    dev = qdev_new("sysbus-m48t08");
> +    qdev_prop_set_int32(dev, "base-year", 1968);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(s, &error_fatal);
> +    sysbus_connect_irq(s, 0, slavio_irq[0]);
> +    sysbus_mmio_map(s, 0, hwdef->nvram_base);
> +    nvram = NVRAM(dev);

While here, can you declare "Nvram *nvram"?

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>   
>       slavio_timer_init_all(hwdef->counter_base, slavio_irq[19], slavio_cpu_irq, smp_cpus);
>   
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/5] sun4u: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 18:27 ` [PATCH 3/5] sun4u: " Mark Cave-Ayland
@ 2020-10-17  9:43   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17  9:43 UTC (permalink / raw)
  To: Mark Cave-Ayland, qemu-devel, qemu-trivial, hpoussin, qemu-ppc,
	atar4qemu, david

On 10/16/20 8:27 PM, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   hw/sparc64/sun4u.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index ad5ca2472a..05e659c8a4 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -671,10 +671,13 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>       pci_ide_create_devs(pci_dev);
>   
>       /* Map NVRAM into I/O (ebus) space */
> -    nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
> -    s = SYS_BUS_DEVICE(nvram);
> +    dev = qdev_new("sysbus-m48t59");
> +    qdev_prop_set_int32(dev, "base-year", 1968);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(s, &error_fatal);
>       memory_region_add_subregion(pci_address_space_io(ebus), 0x2000,
>                                   sysbus_mmio_get_region(s, 0));
> +    nvram = NVRAM(dev);
>    
>       initrd_size = 0;
>       initrd_addr = 0;
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 20:38   ` BALATON Zoltan via
@ 2020-10-17  9:45     ` Philippe Mathieu-Daudé
  2020-10-17 10:44       ` BALATON Zoltan via
  2020-10-17 11:16       ` Mark Cave-Ayland
  2020-10-17 14:41     ` Artyom Tarasenko
  1 sibling, 2 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17  9:45 UTC (permalink / raw)
  To: BALATON Zoltan, Mark Cave-Ayland
  Cc: qemu-trivial, qemu-devel, hpoussin, qemu-ppc, atar4qemu, david

On 10/16/20 10:38 PM, BALATON Zoltan via wrote:
> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/ppc/ppc405_boards.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>> index 6198ec1035..4687715b15 100644
>> --- a/hw/ppc/ppc405_boards.c
>> +++ b/hw/ppc/ppc405_boards.c
>> @@ -28,6 +28,8 @@
>> #include "qemu-common.h"
>> #include "cpu.h"
>> #include "hw/ppc/ppc.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/sysbus.h"
>> #include "ppc405.h"
>> #include "hw/rtc/m48t59.h"
>> #include "hw/block/flash.h"
>> @@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
>>     char *filename;
>>     ppc4xx_bd_info_t bd;
>>     CPUPPCState *env;
>> +    DeviceState *dev;
>> +    SysBusDevice *s;
>>     qemu_irq *pic;
>>     MemoryRegion *bios;
>>     MemoryRegion *sram = g_new(MemoryRegion, 1);
>> @@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
>>     /* Register FPGA */
>>     ref405ep_fpga_init(sysmem, 0xF0300000);
>>     /* Register NVRAM */
>> -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
>> +    dev = qdev_new("sysbus-m48t08");
>> +    qdev_prop_set_int32(dev, "base-year", 1968);
> 
> Is there anything that uses other than 1968 as base year? If not this 
> could be the default in the device so you don't need these set prop 
> calls here and in sun machines.
> 
> The only other place this device is used seems to be ppc/prep machine 
> that uses the isa version but does not set a base year. Is that a bug? 
> The original prep machine removed in b2ce76a0730 used 2000 but that's 
> unlikely as well as these machines predate that.

=)

> Anyway, the sysbus and 
> isa versions are different

They shouldn't, it is the same chipset, wired differently.

> so their default base-year could be set 
> independently and then boards won't need to set this propery and may be 
> could use qdev_create_simple instead or whatever that was renamed to.

Agreed.

Preferably following Zoltan's suggestion:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Regards,
> BALATON Zoltan
> 
>> +    s = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(s, &error_fatal);
>> +    sysbus_mmio_map(s, 0, 0xF0000000);
>>     /* Load kernel */
>>     linux_boot = (kernel_filename != NULL);
>>     if (linux_boot) {
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/5] m48t59: remove legacy m48t59_init() function
  2020-10-16 18:27 ` [PATCH 5/5] m48t59: remove " Mark Cave-Ayland
@ 2020-10-17  9:53   ` Philippe Mathieu-Daudé
  2020-10-17 11:19     ` Mark Cave-Ayland
  0 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17  9:53 UTC (permalink / raw)
  To: Mark Cave-Ayland, qemu-devel, qemu-trivial, hpoussin, qemu-ppc,
	atar4qemu, david

On 10/16/20 8:27 PM, Mark Cave-Ayland wrote:
> Now that all of the callers of this function have been switched to use qdev
> properties, this legacy init function can now be removed.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/rtc/m48t59.c         | 35 -----------------------------------
>   include/hw/rtc/m48t59.h |  4 ----
>   2 files changed, 39 deletions(-)

In the PoC I started after your suggestion, I see:

#define TYPE_M48T02_SRAM "sysbus-m48t02"
#define TYPE_M48T08_SRAM "sysbus-m48t08"
#define TYPE_M48T59_SRAM "sysbus-m48t59"

static void m48t02_class_init(ObjectClass *oc, void *data)
{
     M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc);

     amc->model = 2;
     amc->size = 2 * KiB;
};

static void m48t08_class_init(ObjectClass *oc, void *data)
{
     M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc);

     amc->model = 8;
     amc->size = 8 * KiB;
};

static void m48t59_class_init(ObjectClass *oc, void *data)
{
     M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc);

     amc->model = 59;
     amc->size = 8 * KiB;
};

static const TypeInfo m48t59_register_types[] = {
     {
         .name           = TYPE_M48T02_SRAM,
         .parent         = TYPE_M48TXX_SYSBUS,
         .class_init     = m48t02_class_init,
     }, {
         .name           = TYPE_M48T08_SRAM,
         .parent         = TYPE_M48TXX_SYSBUS,
         .class_init     = m48t08_class_init,
     }, {
         .name           = TYPE_M48T59_SRAM,
         .parent         = TYPE_M48TXX_SYSBUS,
         .class_init     = m48t59_class_init,
     }, {
         .name           = TYPE_M48TXX_SYSBUS,
         .parent         = TYPE_SYS_BUS_DEVICE,
         .instance_size  = sizeof(M48txxSysBusState),
         .instance_init = m48t59_init1,
         .class_size     = sizeof(M48txxSysBusDeviceClass),
         .class_init     = m48txx_sysbus_class_init,
         .abstract       = true,
         .interfaces = (InterfaceInfo[]) {
             { TYPE_NVRAM },
             { }
         }
     }
};

and:

#define TYPE_M48T59_SRAM "isa-m48t59"

static void m48t59_class_init(ObjectClass *oc, void *data)
{
     M48txxISADeviceClass *midc = M48TXX_ISA_CLASS(oc);

     midc->model = 59;
     midc->size = 8 * KiB;
};

static const TypeInfo m48t59_isa_register_types[] = {
     {
         .name           = TYPE_M48T59_SRAM,
         .parent         = TYPE_M48TXX_ISA,
         .class_init     = m48t59_class_init,
     }, {
         .name           = TYPE_M48TXX_ISA,
         .parent         = TYPE_ISA_DEVICE,
         .instance_size  = sizeof(M48txxISAState),
         .class_size     = sizeof(M48txxISADeviceClass),
         .class_init     = m48txx_isa_class_init,
         .abstract       = true,
         .interfaces = (InterfaceInfo[]) {
             { TYPE_NVRAM },
             { }
         }
     }
};

I guess I didn't pursue because I wondered what was the
best way to have the same model usable by sysbus/isa.

IIRC I wanted to proceed as having TYPE_M48T59_SRAM being
an abstract qdev parent, and then TYPE_M48TXX_SYSBUS /
TYPE_M48TXX_ISA implementing the SYSBUS/ISA interfaces.

As it need some thinking I postponed that for after 5.2.

Anyhow back to this patch:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function
  2020-10-17  9:45     ` Philippe Mathieu-Daudé
@ 2020-10-17 10:44       ` BALATON Zoltan via
  2020-10-17 11:53         ` Mark Cave-Ayland
  2020-10-17 11:16       ` Mark Cave-Ayland
  1 sibling, 1 reply; 20+ messages in thread
From: BALATON Zoltan via @ 2020-10-17 10:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-trivial, Mark Cave-Ayland, qemu-devel, hpoussin, qemu-ppc,
	atar4qemu, david

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

On Sat, 17 Oct 2020, Philippe Mathieu-Daudé wrote:
> On 10/16/20 10:38 PM, BALATON Zoltan via wrote:
>> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> hw/ppc/ppc405_boards.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>>> index 6198ec1035..4687715b15 100644
>>> --- a/hw/ppc/ppc405_boards.c
>>> +++ b/hw/ppc/ppc405_boards.c
>>> @@ -28,6 +28,8 @@
>>> #include "qemu-common.h"
>>> #include "cpu.h"
>>> #include "hw/ppc/ppc.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "hw/sysbus.h"
>>> #include "ppc405.h"
>>> #include "hw/rtc/m48t59.h"
>>> #include "hw/block/flash.h"
>>> @@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
>>>     char *filename;
>>>     ppc4xx_bd_info_t bd;
>>>     CPUPPCState *env;
>>> +    DeviceState *dev;
>>> +    SysBusDevice *s;
>>>     qemu_irq *pic;
>>>     MemoryRegion *bios;
>>>     MemoryRegion *sram = g_new(MemoryRegion, 1);
>>> @@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
>>>     /* Register FPGA */
>>>     ref405ep_fpga_init(sysmem, 0xF0300000);
>>>     /* Register NVRAM */
>>> -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
>>> +    dev = qdev_new("sysbus-m48t08");
>>> +    qdev_prop_set_int32(dev, "base-year", 1968);
>> 
>> Is there anything that uses other than 1968 as base year? If not this could 
>> be the default in the device so you don't need these set prop calls here 
>> and in sun machines.
>> 
>> The only other place this device is used seems to be ppc/prep machine that 
>> uses the isa version but does not set a base year. Is that a bug? The 
>> original prep machine removed in b2ce76a0730 used 2000 but that's unlikely 
>> as well as these machines predate that.
>
> =)
>
>> Anyway, the sysbus and isa versions are different
>
> They shouldn't, it is the same chipset, wired differently.

I mean in QEMU the sysbus and isa devices are different object types so 
their default is settable independently. So setting the sysbus device 
base-year does not change the isa device which can be sorted out in 
another patch independently from this series later when the behaviour on 
40p is confirmed.

Regards,
BALATON Zoltan

>> so their default base-year could be set independently and then boards won't 
>> need to set this propery and may be could use qdev_create_simple instead or 
>> whatever that was renamed to.
>
> Agreed.
>
> Preferably following Zoltan's suggestion:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> 
>> Regards,
>> BALATON Zoltan
>> 
>>> +    s = SYS_BUS_DEVICE(dev);
>>> +    sysbus_realize_and_unref(s, &error_fatal);
>>> +    sysbus_mmio_map(s, 0, 0xF0000000);
>>>     /* Load kernel */
>>>     linux_boot = (kernel_filename != NULL);
>>>     if (linux_boot) {
>>> 
>> 
>
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/5] sun4m: use qdev properties instead of legacy m48t59_init() function
  2020-10-17  9:42   ` Philippe Mathieu-Daudé
@ 2020-10-17 11:07     ` Mark Cave-Ayland
  0 siblings, 0 replies; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-17 11:07 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé,
	qemu-devel, qemu-trivial, hpoussin, qemu-ppc, atar4qemu, david

On 17/10/2020 10:42, Philippe Mathieu-Daudé wrote:

> On 10/16/20 8:27 PM, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/sparc/sun4m.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index 54a2b2f9ef..a9bb60f2b2 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -966,7 +966,13 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>>           create_unimplemented_device("SUNW,sx", hwdef->sx_base, 0x2000);
>>       }
>> -    nvram = m48t59_init(slavio_irq[0], hwdef->nvram_base, 0, 0x2000, 1968, 8);
>> +    dev = qdev_new("sysbus-m48t08");
>> +    qdev_prop_set_int32(dev, "base-year", 1968);
>> +    s = SYS_BUS_DEVICE(dev);
>> +    sysbus_realize_and_unref(s, &error_fatal);
>> +    sysbus_connect_irq(s, 0, slavio_irq[0]);
>> +    sysbus_mmio_map(s, 0, hwdef->nvram_base);
>> +    nvram = NVRAM(dev);
> 
> While here, can you declare "Nvram *nvram"?
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>>       slavio_timer_init_all(hwdef->counter_base, slavio_irq[19], slavio_cpu_irq, 
>> smp_cpus);

Yes, that's a good idea. I can fix that up before I apply to my qemu-macppc branc if 
there are no other issues.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function
  2020-10-17  9:45     ` Philippe Mathieu-Daudé
  2020-10-17 10:44       ` BALATON Zoltan via
@ 2020-10-17 11:16       ` Mark Cave-Ayland
  1 sibling, 0 replies; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-17 11:16 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, BALATON Zoltan
  Cc: qemu-trivial, qemu-devel, qemu-ppc, hpoussin, atar4qemu, david

On 17/10/2020 10:45, Philippe Mathieu-Daudé wrote:

> On 10/16/20 10:38 PM, BALATON Zoltan via wrote:
>> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> hw/ppc/ppc405_boards.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>>> index 6198ec1035..4687715b15 100644
>>> --- a/hw/ppc/ppc405_boards.c
>>> +++ b/hw/ppc/ppc405_boards.c
>>> @@ -28,6 +28,8 @@
>>> #include "qemu-common.h"
>>> #include "cpu.h"
>>> #include "hw/ppc/ppc.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "hw/sysbus.h"
>>> #include "ppc405.h"
>>> #include "hw/rtc/m48t59.h"
>>> #include "hw/block/flash.h"
>>> @@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
>>>     char *filename;
>>>     ppc4xx_bd_info_t bd;
>>>     CPUPPCState *env;
>>> +    DeviceState *dev;
>>> +    SysBusDevice *s;
>>>     qemu_irq *pic;
>>>     MemoryRegion *bios;
>>>     MemoryRegion *sram = g_new(MemoryRegion, 1);
>>> @@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
>>>     /* Register FPGA */
>>>     ref405ep_fpga_init(sysmem, 0xF0300000);
>>>     /* Register NVRAM */
>>> -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
>>> +    dev = qdev_new("sysbus-m48t08");
>>> +    qdev_prop_set_int32(dev, "base-year", 1968);
>>
>> Is there anything that uses other than 1968 as base year? If not this could be the 
>> default in the device so you don't need these set prop calls here and in sun machines.
>>
>> The only other place this device is used seems to be ppc/prep machine that uses the 
>> isa version but does not set a base year. Is that a bug? The original prep machine 
>> removed in b2ce76a0730 used 2000 but that's unlikely as well as these machines 
>> predate that.
> 
> =)

This is quite interesting, since if you look at prep.c you see that the machine has 
both a MC146818 RTC and a M48T59 RTC, and the NVRAM variables are stored in the 
M48T59. My guess is that a real 40p has a MC146818, but OHW only supported M48T59 and 
so it was only ever used to pass data from QEMU to OHW - the OS would use MC146818 
for the clock which has the base year set correctly.

>> Anyway, the sysbus and isa versions are different
> 
> They shouldn't, it is the same chipset, wired differently.
> 
>> so their default base-year could be set independently and then boards won't need to 
>> set this propery and may be could use qdev_create_simple instead or whatever that 
>> was renamed to.
> 
> Agreed.
> 
> Preferably following Zoltan's suggestion:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

There is certainly some more untangling to be done here, but given my current backlog 
it is something that will need to wait until after 5.2.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/5] m48t59: remove legacy m48t59_init() function
  2020-10-17  9:53   ` Philippe Mathieu-Daudé
@ 2020-10-17 11:19     ` Mark Cave-Ayland
  2020-10-17 13:57       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-17 11:19 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé,
	qemu-devel, qemu-trivial, hpoussin, qemu-ppc, atar4qemu, david

On 17/10/2020 10:53, Philippe Mathieu-Daudé wrote:

> On 10/16/20 8:27 PM, Mark Cave-Ayland wrote:
>> Now that all of the callers of this function have been switched to use qdev
>> properties, this legacy init function can now be removed.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/rtc/m48t59.c         | 35 -----------------------------------
>>   include/hw/rtc/m48t59.h |  4 ----
>>   2 files changed, 39 deletions(-)
> 
> In the PoC I started after your suggestion, I see:
> 
> #define TYPE_M48T02_SRAM "sysbus-m48t02"
> #define TYPE_M48T08_SRAM "sysbus-m48t08"
> #define TYPE_M48T59_SRAM "sysbus-m48t59"
> 
> static void m48t02_class_init(ObjectClass *oc, void *data)
> {
>      M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc);
> 
>      amc->model = 2;
>      amc->size = 2 * KiB;
> };
> 
> static void m48t08_class_init(ObjectClass *oc, void *data)
> {
>      M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc);
> 
>      amc->model = 8;
>      amc->size = 8 * KiB;
> };
> 
> static void m48t59_class_init(ObjectClass *oc, void *data)
> {
>      M48txxSysBusDeviceClass *amc = M48TXX_SYS_BUS_CLASS(oc);
> 
>      amc->model = 59;
>      amc->size = 8 * KiB;
> };
> 
> static const TypeInfo m48t59_register_types[] = {
>      {
>          .name           = TYPE_M48T02_SRAM,
>          .parent         = TYPE_M48TXX_SYSBUS,
>          .class_init     = m48t02_class_init,
>      }, {
>          .name           = TYPE_M48T08_SRAM,
>          .parent         = TYPE_M48TXX_SYSBUS,
>          .class_init     = m48t08_class_init,
>      }, {
>          .name           = TYPE_M48T59_SRAM,
>          .parent         = TYPE_M48TXX_SYSBUS,
>          .class_init     = m48t59_class_init,
>      }, {
>          .name           = TYPE_M48TXX_SYSBUS,
>          .parent         = TYPE_SYS_BUS_DEVICE,
>          .instance_size  = sizeof(M48txxSysBusState),
>          .instance_init = m48t59_init1,
>          .class_size     = sizeof(M48txxSysBusDeviceClass),
>          .class_init     = m48txx_sysbus_class_init,
>          .abstract       = true,
>          .interfaces = (InterfaceInfo[]) {
>              { TYPE_NVRAM },
>              { }
>          }
>      }
> };
> 
> and:
> 
> #define TYPE_M48T59_SRAM "isa-m48t59"
> 
> static void m48t59_class_init(ObjectClass *oc, void *data)
> {
>      M48txxISADeviceClass *midc = M48TXX_ISA_CLASS(oc);
> 
>      midc->model = 59;
>      midc->size = 8 * KiB;
> };
> 
> static const TypeInfo m48t59_isa_register_types[] = {
>      {
>          .name           = TYPE_M48T59_SRAM,
>          .parent         = TYPE_M48TXX_ISA,
>          .class_init     = m48t59_class_init,
>      }, {
>          .name           = TYPE_M48TXX_ISA,
>          .parent         = TYPE_ISA_DEVICE,
>          .instance_size  = sizeof(M48txxISAState),
>          .class_size     = sizeof(M48txxISADeviceClass),
>          .class_init     = m48txx_isa_class_init,
>          .abstract       = true,
>          .interfaces = (InterfaceInfo[]) {
>              { TYPE_NVRAM },
>              { }
>          }
>      }
> };
> 
> I guess I didn't pursue because I wondered what was the
> best way to have the same model usable by sysbus/isa.
> 
> IIRC I wanted to proceed as having TYPE_M48T59_SRAM being
> an abstract qdev parent, and then TYPE_M48TXX_SYSBUS /
> TYPE_M48TXX_ISA implementing the SYSBUS/ISA interfaces.
> 
> As it need some thinking I postponed that for after 5.2.
> 
> Anyhow back to this patch:
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Ha indeed, I think you also came to the same conclusion that I did in my previous 
email :)

I'm also not convinced by the dynamic generation of various M48TXX types using 
class_data - this seems overly complex, and there's nothing there I can see that 
can't be just as easily handled using qdev properties using an abstract parent as you 
suggest above.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function
  2020-10-17 10:44       ` BALATON Zoltan via
@ 2020-10-17 11:53         ` Mark Cave-Ayland
  0 siblings, 0 replies; 20+ messages in thread
From: Mark Cave-Ayland @ 2020-10-17 11:53 UTC (permalink / raw)
  To: BALATON Zoltan, Philippe Mathieu-Daudé
  Cc: qemu-trivial, qemu-devel, qemu-ppc, hpoussin, atar4qemu, david

On 17/10/2020 11:44, BALATON Zoltan via wrote:

> On Sat, 17 Oct 2020, Philippe Mathieu-Daudé wrote:
>> On 10/16/20 10:38 PM, BALATON Zoltan via wrote:
>>> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> hw/ppc/ppc405_boards.c | 10 +++++++++-
>>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>>>> index 6198ec1035..4687715b15 100644
>>>> --- a/hw/ppc/ppc405_boards.c
>>>> +++ b/hw/ppc/ppc405_boards.c
>>>> @@ -28,6 +28,8 @@
>>>> #include "qemu-common.h"
>>>> #include "cpu.h"
>>>> #include "hw/ppc/ppc.h"
>>>> +#include "hw/qdev-properties.h"
>>>> +#include "hw/sysbus.h"
>>>> #include "ppc405.h"
>>>> #include "hw/rtc/m48t59.h"
>>>> #include "hw/block/flash.h"
>>>> @@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
>>>>     char *filename;
>>>>     ppc4xx_bd_info_t bd;
>>>>     CPUPPCState *env;
>>>> +    DeviceState *dev;
>>>> +    SysBusDevice *s;
>>>>     qemu_irq *pic;
>>>>     MemoryRegion *bios;
>>>>     MemoryRegion *sram = g_new(MemoryRegion, 1);
>>>> @@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
>>>>     /* Register FPGA */
>>>>     ref405ep_fpga_init(sysmem, 0xF0300000);
>>>>     /* Register NVRAM */
>>>> -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
>>>> +    dev = qdev_new("sysbus-m48t08");
>>>> +    qdev_prop_set_int32(dev, "base-year", 1968);
>>>
>>> Is there anything that uses other than 1968 as base year? If not this could be the 
>>> default in the device so you don't need these set prop calls here and in sun 
>>> machines.
>>>
>>> The only other place this device is used seems to be ppc/prep machine that uses 
>>> the isa version but does not set a base year. Is that a bug? The original prep 
>>> machine removed in b2ce76a0730 used 2000 but that's unlikely as well as these 
>>> machines predate that.
>>
>> =)
>>
>>> Anyway, the sysbus and isa versions are different
>>
>> They shouldn't, it is the same chipset, wired differently.
> 
> I mean in QEMU the sysbus and isa devices are different object types so their default 
> is settable independently. So setting the sysbus device base-year does not change the 
> isa device which can be sorted out in another patch independently from this series 
> later when the behaviour on 40p is confirmed.

Right, there are certainly some questions around exactly how this behaviour works but 
in general people seem happy with this series. I'm going to apply this to my 
qemu-macppc branch with the NVRAM cast suggested by Philippe so the basic conversion 
is done, and then other improvements/tidy-ups can follow up later as time allows.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/5] m48t59: remove legacy m48t59_init() function
  2020-10-17 11:19     ` Mark Cave-Ayland
@ 2020-10-17 13:57       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17 13:57 UTC (permalink / raw)
  To: Mark Cave-Ayland, qemu-devel, qemu-trivial, hpoussin, qemu-ppc,
	atar4qemu, david

On 10/17/20 1:19 PM, Mark Cave-Ayland wrote:
> On 17/10/2020 10:53, Philippe Mathieu-Daudé wrote:
> 
>> On 10/16/20 8:27 PM, Mark Cave-Ayland wrote:
>>> Now that all of the callers of this function have been switched to 
>>> use qdev
>>> properties, this legacy init function can now be removed.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/rtc/m48t59.c         | 35 -----------------------------------
>>>   include/hw/rtc/m48t59.h |  4 ----
>>>   2 files changed, 39 deletions(-)
...

>> static void m48t59_class_init(ObjectClass *oc, void *data)
>> {
>>      M48txxISADeviceClass *midc = M48TXX_ISA_CLASS(oc);
>>
>>      midc->model = 59;
>>      midc->size = 8 * KiB;
>> };
>>
>> static const TypeInfo m48t59_isa_register_types[] = {
>>      {
>>          .name           = TYPE_M48T59_SRAM,
>>          .parent         = TYPE_M48TXX_ISA,
>>          .class_init     = m48t59_class_init,
>>      }, {
>>          .name           = TYPE_M48TXX_ISA,
>>          .parent         = TYPE_ISA_DEVICE,
>>          .instance_size  = sizeof(M48txxISAState),
>>          .class_size     = sizeof(M48txxISADeviceClass),
>>          .class_init     = m48txx_isa_class_init,
>>          .abstract       = true,
>>          .interfaces = (InterfaceInfo[]) {
>>              { TYPE_NVRAM },
>>              { }
>>          }
>>      }
>> };
>>
>> I guess I didn't pursue because I wondered what was the
>> best way to have the same model usable by sysbus/isa.
>>
>> IIRC I wanted to proceed as having TYPE_M48T59_SRAM being
>> an abstract qdev parent, and then TYPE_M48TXX_SYSBUS /
>> TYPE_M48TXX_ISA implementing the SYSBUS/ISA interfaces.
>>
>> As it need some thinking I postponed that for after 5.2.
>>
>> Anyhow back to this patch:
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Ha indeed, I think you also came to the same conclusion that I did in my 
> previous email :)
> 
> I'm also not convinced by the dynamic generation of various M48TXX types 
> using class_data - this seems overly complex, and there's nothing there 
> I can see that can't be just as easily handled using qdev properties 
> using an abstract parent as you suggest above.

Yeah, no advantage except having uniform code style that serves
as example.

> 
> 
> ATB,
> 
> Mark.
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/5] ppc405_boards: use qdev properties instead of legacy m48t59_init() function
  2020-10-16 20:38   ` BALATON Zoltan via
  2020-10-17  9:45     ` Philippe Mathieu-Daudé
@ 2020-10-17 14:41     ` Artyom Tarasenko
  1 sibling, 0 replies; 20+ messages in thread
From: Artyom Tarasenko @ 2020-10-17 14:41 UTC (permalink / raw)
  To: BALATON Zoltan
  Cc: qemu-trivial, Mark Cave-Ayland, qemu-devel, f4bug,
	Hervé Poussineau, qemu-ppc, david

On Fri, Oct 16, 2020 at 10:38 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> > hw/ppc/ppc405_boards.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> > index 6198ec1035..4687715b15 100644
> > --- a/hw/ppc/ppc405_boards.c
> > +++ b/hw/ppc/ppc405_boards.c
> > @@ -28,6 +28,8 @@
> > #include "qemu-common.h"
> > #include "cpu.h"
> > #include "hw/ppc/ppc.h"
> > +#include "hw/qdev-properties.h"
> > +#include "hw/sysbus.h"
> > #include "ppc405.h"
> > #include "hw/rtc/m48t59.h"
> > #include "hw/block/flash.h"
> > @@ -145,6 +147,8 @@ static void ref405ep_init(MachineState *machine)
> >     char *filename;
> >     ppc4xx_bd_info_t bd;
> >     CPUPPCState *env;
> > +    DeviceState *dev;
> > +    SysBusDevice *s;
> >     qemu_irq *pic;
> >     MemoryRegion *bios;
> >     MemoryRegion *sram = g_new(MemoryRegion, 1);
> > @@ -227,7 +231,11 @@ static void ref405ep_init(MachineState *machine)
> >     /* Register FPGA */
> >     ref405ep_fpga_init(sysmem, 0xF0300000);
> >     /* Register NVRAM */
> > -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
> > +    dev = qdev_new("sysbus-m48t08");
> > +    qdev_prop_set_int32(dev, "base-year", 1968);
>
> Is there anything that uses other than 1968 as base year?

I think 40p uses 1900 as the base year.

> If not this
> could be the default in the device so you don't need these set prop calls
> here and in sun machines.
>
> The only other place this device is used seems to be ppc/prep machine that
> uses the isa version but does not set a base year. Is that a bug? The
> original prep machine removed in b2ce76a0730 used 2000 but that's unlikely
> as well as these machines predate that. Anyway, the sysbus and isa
> versions are different so their default base-year could be set
> independently and then boards won't need to set this propery and may be
> could use qdev_create_simple instead or whatever that was renamed to.
>
> Regards,
> BALATON Zoltan
>
> > +    s = SYS_BUS_DEVICE(dev);
> > +    sysbus_realize_and_unref(s, &error_fatal);
> > +    sysbus_mmio_map(s, 0, 0xF0000000);
> >     /* Load kernel */
> >     linux_boot = (kernel_filename != NULL);
> >     if (linux_boot) {
> >


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-10-17 14:44 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 18:27 [PATCH 0/5] m48t59: remove legacy init functions Mark Cave-Ayland
2020-10-16 18:27 ` [PATCH 1/5] m48t59-isa: remove legacy m48t59_init_isa() function Mark Cave-Ayland
2020-10-17  9:39   ` Philippe Mathieu-Daudé
2020-10-16 18:27 ` [PATCH 2/5] sun4m: use qdev properties instead of legacy m48t59_init() function Mark Cave-Ayland
2020-10-17  9:42   ` Philippe Mathieu-Daudé
2020-10-17 11:07     ` Mark Cave-Ayland
2020-10-16 18:27 ` [PATCH 3/5] sun4u: " Mark Cave-Ayland
2020-10-17  9:43   ` Philippe Mathieu-Daudé
2020-10-16 18:27 ` [PATCH 4/5] ppc405_boards: " Mark Cave-Ayland
2020-10-16 20:38   ` BALATON Zoltan via
2020-10-17  9:45     ` Philippe Mathieu-Daudé
2020-10-17 10:44       ` BALATON Zoltan via
2020-10-17 11:53         ` Mark Cave-Ayland
2020-10-17 11:16       ` Mark Cave-Ayland
2020-10-17 14:41     ` Artyom Tarasenko
2020-10-16 18:27 ` [PATCH 5/5] m48t59: remove " Mark Cave-Ayland
2020-10-17  9:53   ` Philippe Mathieu-Daudé
2020-10-17 11:19     ` Mark Cave-Ayland
2020-10-17 13:57       ` Philippe Mathieu-Daudé
2020-10-16 20:21 ` [PATCH 0/5] m48t59: remove legacy init functions Hervé Poussineau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).