[PATCH 5/6] ppc405_boards: use qdev instead of legacy m48t59_init() function

[PATCH 5/6] ppc405_boards: use qdev instead of legacy m48t59_init() function

[BALATON Zoltan via]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc405_boards.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 6198ec1035..7a11a38831 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -28,6 +28,7 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "hw/ppc/ppc.h"
+#include "hw/sysbus.h"
 #include "ppc405.h"
 #include "hw/rtc/m48t59.h"
 #include "hw/block/flash.h"
@@ -227,7 +228,7 @@ static void ref405ep_init(MachineState *machine)
     /* Register FPGA */
     ref405ep_fpga_init(sysmem, 0xF0300000);
     /* Register NVRAM */
-    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
+    sysbus_create_simple("sysbus-m48t08", 0xF0000000, NULL);
     /* Load kernel */
     linux_boot = (kernel_filename != NULL);
     if (linux_boot) {
-- 
2.21.3

[PATCH 2/6] mt48t59: Set default value of base-year property to 1968

[BALATON Zoltan via]

All instances set this value explicitely so make it the default to
make it simpler to create instances without setting property.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/rtc/m48t59.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
index 6525206976..3108cf3d3f 100644
--- a/hw/rtc/m48t59.c
+++ b/hw/rtc/m48t59.c
@@ -655,7 +655,7 @@ static void m48txx_sysbus_toggle_lock(Nvram *obj, int lock)
 }
 
 static Property m48t59_sysbus_properties[] = {
-    DEFINE_PROP_INT32("base-year", M48txxSysBusState, state.base_year, 0),
+    DEFINE_PROP_INT32("base-year", M48txxSysBusState, state.base_year, 1968),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.21.3

[PATCH 4/6] sun4u: use qdev instead of legacy m48t59_init() function

[BALATON Zoltan via]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/sparc64/sun4u.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index ad5ca2472a..a89ebed6f0 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -671,10 +671,12 @@ 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");
+    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.21.3

[PATCH 6/6] m48t59: remove legacy m48t59_init() function

[BALATON Zoltan via]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

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>
Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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 3108cf3d3f..e74716c491 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.21.3

[PATCH 1/6] m48t59-isa: remove legacy m48t59_init_isa() function

[BALATON Zoltan via]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

This function is no longer used within the codebase.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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.21.3

[PATCH 0/6] m48t59: remove legacy init functions

[BALATON Zoltan via]

This is inspired by Mark's series:

https://lists.nongnu.org/archive/html/qemu-ppc/2020-10/msg00251.html

and implements what I've suggested in review of that series to
simplify it and avoid code churn if implementing my suggestion later.

Regards,
BALATON Zoltan

BALATON Zoltan (4):
  mt48t59: Set default value of base-year property to 1968
  sun4m: use qdev instead of legacy m48t59_init() function
  sun4u: use qdev instead of legacy m48t59_init() function
  ppc405_boards: use qdev instead of legacy m48t59_init() function

Mark Cave-Ayland (2):
  m48t59-isa: remove legacy m48t59_init_isa() function
  m48t59: remove legacy m48t59_init() function

 hw/ppc/ppc405_boards.c  |  3 ++-
 hw/rtc/m48t59-isa.c     | 25 -------------------------
 hw/rtc/m48t59.c         | 37 +------------------------------------
 hw/sparc/sun4m.c        |  5 +++--
 hw/sparc64/sun4u.c      |  6 ++++--
 include/hw/rtc/m48t59.h |  6 ------
 6 files changed, 10 insertions(+), 72 deletions(-)

-- 
2.21.3

[PATCH 3/6] sun4m: use qdev instead of legacy m48t59_init() function

[BALATON Zoltan via]

Also declare nvram variable with the correct type.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/sparc/sun4m.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 54a2b2f9ef..7f1a48440c 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -837,7 +837,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
 {
     DeviceState *slavio_intctl;
     unsigned int i;
-    void *nvram;
+    Nvram *nvram;
     qemu_irq *cpu_irqs[MAX_CPUS], slavio_irq[32], slavio_cpu_irq[MAX_CPUS];
     qemu_irq fdc_tc;
     unsigned long kernel_size;
@@ -966,7 +966,8 @@ 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);
+    nvram = NVRAM(sysbus_create_simple("sysbus-m48t08", hwdef->nvram_base,
+                                       slavio_irq[0]));
 
     slavio_timer_init_all(hwdef->counter_base, slavio_irq[19], slavio_cpu_irq, smp_cpus);
 
-- 
2.21.3

Re: [PATCH 2/6] mt48t59: Set default value of base-year property to 1968

[Philippe Mathieu-Daudé]

On 10/17/20 10:13 PM, BALATON Zoltan via wrote:
> All instances set this value explicitely so make it the default to

Typo explicitely -> explicitly.

> make it simpler to create instances without setting property.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

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

> ---
>   hw/rtc/m48t59.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
> index 6525206976..3108cf3d3f 100644
> --- a/hw/rtc/m48t59.c
> +++ b/hw/rtc/m48t59.c
> @@ -655,7 +655,7 @@ static void m48txx_sysbus_toggle_lock(Nvram *obj, int lock)
>   }
>   
>   static Property m48t59_sysbus_properties[] = {
> -    DEFINE_PROP_INT32("base-year", M48txxSysBusState, state.base_year, 0),
> +    DEFINE_PROP_INT32("base-year", M48txxSysBusState, state.base_year, 1968),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> 

Re: [PATCH 3/6] sun4m: use qdev instead of legacy m48t59_init() function

[Philippe Mathieu-Daudé]

On 10/17/20 10:13 PM, BALATON Zoltan via wrote:
> Also declare nvram variable with the correct type.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

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

> ---
>   hw/sparc/sun4m.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 54a2b2f9ef..7f1a48440c 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -837,7 +837,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>   {
>       DeviceState *slavio_intctl;
>       unsigned int i;
> -    void *nvram;
> +    Nvram *nvram;
>       qemu_irq *cpu_irqs[MAX_CPUS], slavio_irq[32], slavio_cpu_irq[MAX_CPUS];
>       qemu_irq fdc_tc;
>       unsigned long kernel_size;
> @@ -966,7 +966,8 @@ 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);
> +    nvram = NVRAM(sysbus_create_simple("sysbus-m48t08", hwdef->nvram_base,
> +                                       slavio_irq[0]));
>   
>       slavio_timer_init_all(hwdef->counter_base, slavio_irq[19], slavio_cpu_irq, smp_cpus);
>   
> 

Re: [PATCH 5/6] ppc405_boards: use qdev instead of legacy m48t59_init() function

[Philippe Mathieu-Daudé]

On 10/17/20 10:13 PM, BALATON Zoltan via wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/ppc405_boards.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

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

> 
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 6198ec1035..7a11a38831 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -28,6 +28,7 @@
>   #include "qemu-common.h"
>   #include "cpu.h"
>   #include "hw/ppc/ppc.h"
> +#include "hw/sysbus.h"
>   #include "ppc405.h"
>   #include "hw/rtc/m48t59.h"
>   #include "hw/block/flash.h"
> @@ -227,7 +228,7 @@ static void ref405ep_init(MachineState *machine)
>       /* Register FPGA */
>       ref405ep_fpga_init(sysmem, 0xF0300000);
>       /* Register NVRAM */
> -    m48t59_init(NULL, 0xF0000000, 0, 8192, 1968, 8);
> +    sysbus_create_simple("sysbus-m48t08", 0xF0000000, NULL);
>       /* Load kernel */
>       linux_boot = (kernel_filename != NULL);
>       if (linux_boot) {
> 

Re: [PATCH 4/6] sun4u: use qdev instead of legacy m48t59_init() function

[Philippe Mathieu-Daudé]

On 10/17/20 10:13 PM, BALATON Zoltan via wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/sparc64/sun4u.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index ad5ca2472a..a89ebed6f0 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -671,10 +671,12 @@ 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");
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_realize_and_unref(s, &error_fatal);

I'd use &error_abort here (so if that ever happens, it is
easier to debug it), otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>       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;
> 

Re: [PATCH 0/6] m48t59: remove legacy init functions

[Mark Cave-Ayland]

On 17/10/2020 21:13, BALATON Zoltan via wrote:

> This is inspired by Mark's series:
> 
> https://lists.nongnu.org/archive/html/qemu-ppc/2020-10/msg00251.html
> 
> and implements what I've suggested in review of that series to
> simplify it and avoid code churn if implementing my suggestion later.
> 
> Regards,
> BALATON Zoltan
> 
> BALATON Zoltan (4):
>    mt48t59: Set default value of base-year property to 1968
>    sun4m: use qdev instead of legacy m48t59_init() function
>    sun4u: use qdev instead of legacy m48t59_init() function
>    ppc405_boards: use qdev instead of legacy m48t59_init() function
> 
> Mark Cave-Ayland (2):
>    m48t59-isa: remove legacy m48t59_init_isa() function
>    m48t59: remove legacy m48t59_init() function
> 
>   hw/ppc/ppc405_boards.c  |  3 ++-
>   hw/rtc/m48t59-isa.c     | 25 -------------------------
>   hw/rtc/m48t59.c         | 37 +------------------------------------
>   hw/sparc/sun4m.c        |  5 +++--
>   hw/sparc64/sun4u.c      |  6 ++++--
>   include/hw/rtc/m48t59.h |  6 ------
>   6 files changed, 10 insertions(+), 72 deletions(-)

Unfortunately this arrived too late - I'd already finished the tagging and local 
testing, but didn't get a chance to do the final PR before having to head out yesterday.

I think most people here agree that this code could be improved, but I'm not clear 
that this is the right solution given that Artyom has already pointed out that 40p 
uses 1900 as the base year. There would also be an overlap with the ideas that 
Philippe has expressed in this thread which would cause more code churn later, so if 
this is something that interests you I would suggest starting a separate thread to 
gain consensus as to the desired solution first before working on an updated series.


ATB,

Mark.