Am 22. Mai 2022 12:13:48 UTC schrieb Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>: >On 22/05/2022 10:07, Bernhard Beschow wrote: > >> On Sat, May 21, 2022 at 11:24 AM Mark Cave-Ayland >> <mark.cave-ayl...@ilande.co.uk <mailto: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 >> <mailto: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. > >True, that's not a completely unreasonable argument. In that case how about >moving the RTC_ISA_BASE define to somewhere around the top of >hw/rtc/mc146818rtc.c so that the origin of the 0x70 address is preserved?
Okay, I'll move it there. > >> > 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 >> <mailto:mark.cave-ayl...@ilande.co.uk>> >> >> >> ATB, >> >> Mark. > > >ATB, > >Mark.
