Re: [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset

[Mark Cave-Ayland]

On 20/05/2022 18:45, Bernhard Beschow wrote:

> Exposing the io_base offset as a QOM property not only allows it to be
> configurable but also to be displayed in HMP:
>
> Before:
>
> (qemu) info qtree
>         ...
>            dev: mc146818rtc, id ""
>              gpio-out "" 1
>              base_year = 0 (0x0)
>              irq = 8 (0x8)
>              lost_tick_policy = "discard"
>
> After:
>
>            dev: mc146818rtc, id ""
>              gpio-out "" 1
>              base_year = 0 (0x0)
>              iobase = 112 (0x70)
>              irq = 8 (0x8)
>              lost_tick_policy = "discard"
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/microvm-dt.c         | 2 +-
>   hw/rtc/mc146818rtc.c         | 7 ++++---
>   include/hw/rtc/mc146818rtc.h | 2 +-
>   3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c
> index a5db9e4e5a..39fe425b26 100644
> --- a/hw/i386/microvm-dt.c
> +++ b/hw/i386/microvm-dt.c
> @@ -209,7 +209,7 @@ static void dt_add_isa_rtc(MicrovmMachineState *mms, 
> ISADevice *dev)
>   {
>       const char compat[] = "motorola,mc146818";
>       uint32_t irq = object_property_get_uint(OBJECT(dev), "irq", NULL);
> -    hwaddr base = RTC_ISA_BASE;
> +    hwaddr base = object_property_get_int(OBJECT(dev), "iobase", NULL);

Same comment here re: &error_abort.

>       hwaddr size = 8;
>       char *nodename;
>   
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index f235c2ddbe..484f91b6f8 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -941,7 +941,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>       qemu_register_suspend_notifier(&s->suspend_notifier);
>   
>       memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2);
> -    isa_register_ioport(isadev, &s->io, RTC_ISA_BASE);
> +    isa_register_ioport(isadev, &s->io, s->io_base);
>   
>       /* register rtc 0x70 port for coalesced_pio */
>       memory_region_set_flush_coalesced(&s->io);
> @@ -950,7 +950,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>       memory_region_add_subregion(&s->io, 0, &s->coalesced_io);
>       memory_region_add_coalescing(&s->coalesced_io, 0, 1);
>   
> -    qdev_set_legacy_instance_id(dev, RTC_ISA_BASE, 3);
> +    qdev_set_legacy_instance_id(dev, s->io_base, 3);
>   
>       object_property_add_tm(OBJECT(s), "date", rtc_get_date);
>   
> @@ -983,6 +983,7 @@ ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
>   
>   static Property mc146818rtc_properties[] = {
>       DEFINE_PROP_INT32("base_year", RTCState, base_year, 1980),
> +    DEFINE_PROP_UINT32("iobase", RTCState, io_base, 0x70),

I think this should be RTC_ISA_BASE?

>       DEFINE_PROP_UINT8("irq", RTCState, isairq, RTC_ISA_IRQ),
>       DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", RTCState,
>                                  lost_tick_policy, LOST_TICK_POLICY_DISCARD),
> @@ -1028,7 +1029,7 @@ static void rtc_build_aml(ISADevice *isadev, Aml *scope)
>        * does, even though qemu only responds to the first two ports.
>        */
>       crs = aml_resource_template();
> -    aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
> +    aml_append(crs, aml_io(AML_DECODE16, s->io_base, s->io_base,
>                              0x01, 0x08));
>       aml_append(crs, aml_irq_no_flags(s->isairq));
>   
> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> index 33d85753c0..1f7942a9f8 100644
> --- a/include/hw/rtc/mc146818rtc.h
> +++ b/include/hw/rtc/mc146818rtc.h
> @@ -26,6 +26,7 @@ struct RTCState {
>       uint8_t cmos_data[128];
>       uint8_t cmos_index;
>       uint8_t isairq;
> +    uint32_t io_base;
>       int32_t base_year;
>       uint64_t base_rtc;
>       uint64_t last_update;
> @@ -49,7 +50,6 @@ struct RTCState {
>   };
>   
>   #define RTC_ISA_IRQ 8
> -#define RTC_ISA_BASE 0x70

... and so you'll need to keep this.

>   ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
>                                qemu_irq intercept_irq);

Otherwise:

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


ATB,

Mark.