On Sat, May 21, 2022 at 11:24 AM Mark Cave-Ayland < mark.cave-ayl...@ilande.co.uk> wrote:
> 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 <shen...@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-ayl...@ilande.co.uk> > > > ATB, > > Mark. >