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.
Ack.
> 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.
My intention was indeed to remove it since it is now redundant.
Keeping the constant public has the risk of using it instead of the user-configurable QOM property which could cause bugs.
> 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.