* [PATCH 0/2] QOM improvements for rtc/mc146818rtc
@ 2022-05-20 17:45 Bernhard Beschow
2022-05-20 17:45 ` [PATCH 1/2] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property Bernhard Beschow
2022-05-20 17:45 ` [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset Bernhard Beschow
0 siblings, 2 replies; 9+ messages in thread
From: Bernhard Beschow @ 2022-05-20 17:45 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Bernhard Beschow
This little series enhances QOM support for mc146818rtc:
* makes microvm-dt respect mc146818rtc's IRQ number set by QOM property and
* adds an io_base QOM property similar to other ISA devices
Bernhard Beschow (2):
hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM
property
rtc/mc146818rtc: QOM'ify io_base offset
hw/i386/microvm-dt.c | 4 ++--
hw/rtc/mc146818rtc.c | 7 ++++---
include/hw/rtc/mc146818rtc.h | 2 +-
3 files changed, 7 insertions(+), 6 deletions(-)
--
2.36.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property 2022-05-20 17:45 [PATCH 0/2] QOM improvements for rtc/mc146818rtc Bernhard Beschow @ 2022-05-20 17:45 ` Bernhard Beschow 2022-05-21 9:19 ` Mark Cave-Ayland 2022-05-20 17:45 ` [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset Bernhard Beschow 1 sibling, 1 reply; 9+ messages in thread From: Bernhard Beschow @ 2022-05-20 17:45 UTC (permalink / raw) To: qemu-devel Cc: qemu-trivial, Bernhard Beschow, Paolo Bonzini, Richard Henderson, Eduardo Habkost, Michael S. Tsirkin, Marcel Apfelbaum Since commit 3b004a16540aa41f2aa6a1ceb0bf306716766914 'hw/rtc/ mc146818rtc: QOM'ify IRQ number' mc146818rtc's IRQ number is configurable. Fix microvm-dt to respect its value. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/i386/microvm-dt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c index 9c3c4995b4..a5db9e4e5a 100644 --- a/hw/i386/microvm-dt.c +++ b/hw/i386/microvm-dt.c @@ -208,7 +208,7 @@ static void dt_add_isa_serial(MicrovmMachineState *mms, ISADevice *dev) static void dt_add_isa_rtc(MicrovmMachineState *mms, ISADevice *dev) { const char compat[] = "motorola,mc146818"; - uint32_t irq = RTC_ISA_IRQ; + uint32_t irq = object_property_get_uint(OBJECT(dev), "irq", NULL); hwaddr base = RTC_ISA_BASE; hwaddr size = 8; char *nodename; -- 2.36.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property 2022-05-20 17:45 ` [PATCH 1/2] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property Bernhard Beschow @ 2022-05-21 9:19 ` Mark Cave-Ayland 2022-05-22 8:56 ` Bernhard Beschow 0 siblings, 1 reply; 9+ messages in thread From: Mark Cave-Ayland @ 2022-05-21 9:19 UTC (permalink / raw) To: Bernhard Beschow, qemu-devel Cc: qemu-trivial, Paolo Bonzini, Richard Henderson, Eduardo Habkost, Michael S. Tsirkin, Marcel Apfelbaum On 20/05/2022 18:45, Bernhard Beschow wrote: > Since commit 3b004a16540aa41f2aa6a1ceb0bf306716766914 'hw/rtc/ > mc146818rtc: QOM'ify IRQ number' mc146818rtc's IRQ number is > configurable. Fix microvm-dt to respect its value. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/i386/microvm-dt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c > index 9c3c4995b4..a5db9e4e5a 100644 > --- a/hw/i386/microvm-dt.c > +++ b/hw/i386/microvm-dt.c > @@ -208,7 +208,7 @@ static void dt_add_isa_serial(MicrovmMachineState *mms, ISADevice *dev) > static void dt_add_isa_rtc(MicrovmMachineState *mms, ISADevice *dev) > { > const char compat[] = "motorola,mc146818"; > - uint32_t irq = RTC_ISA_IRQ; > + uint32_t irq = object_property_get_uint(OBJECT(dev), "irq", NULL); > hwaddr base = RTC_ISA_BASE; > hwaddr size = 8; > char *nodename; Rather than using NULL as the last parameter to object_property_get_uint() I think using &error_abort to force an explicit failure if the irq property doesn't exist would be better. Otherwise: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property 2022-05-21 9:19 ` Mark Cave-Ayland @ 2022-05-22 8:56 ` Bernhard Beschow 0 siblings, 0 replies; 9+ messages in thread From: Bernhard Beschow @ 2022-05-22 8:56 UTC (permalink / raw) To: Mark Cave-Ayland Cc: QEMU Developers, QEMU Trivial, Paolo Bonzini, Richard Henderson, Eduardo Habkost, Michael S. Tsirkin, Marcel Apfelbaum [-- Attachment #1: Type: text/plain, Size: 1467 bytes --] On Sat, May 21, 2022 at 11:20 AM Mark Cave-Ayland < mark.cave-ayland@ilande.co.uk> wrote: > On 20/05/2022 18:45, Bernhard Beschow wrote: > > > Since commit 3b004a16540aa41f2aa6a1ceb0bf306716766914 'hw/rtc/ > > mc146818rtc: QOM'ify IRQ number' mc146818rtc's IRQ number is > > configurable. Fix microvm-dt to respect its value. > > > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > > --- > > hw/i386/microvm-dt.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c > > index 9c3c4995b4..a5db9e4e5a 100644 > > --- a/hw/i386/microvm-dt.c > > +++ b/hw/i386/microvm-dt.c > > @@ -208,7 +208,7 @@ static void dt_add_isa_serial(MicrovmMachineState > *mms, ISADevice *dev) > > static void dt_add_isa_rtc(MicrovmMachineState *mms, ISADevice *dev) > > { > > const char compat[] = "motorola,mc146818"; > > - uint32_t irq = RTC_ISA_IRQ; > > + uint32_t irq = object_property_get_uint(OBJECT(dev), "irq", NULL); > > hwaddr base = RTC_ISA_BASE; > > hwaddr size = 8; > > char *nodename; > > Rather than using NULL as the last parameter to object_property_get_uint() > I think > using &error_abort to force an explicit failure if the irq property > doesn't exist > would be better. > Ack. I'll then also fix dt_add_isa_serial() in a dedicated commit for consistency. Otherwise: > > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > ATB, > > Mark. > [-- Attachment #2: Type: text/html, Size: 2387 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset 2022-05-20 17:45 [PATCH 0/2] QOM improvements for rtc/mc146818rtc Bernhard Beschow 2022-05-20 17:45 ` [PATCH 1/2] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property Bernhard Beschow @ 2022-05-20 17:45 ` Bernhard Beschow 2022-05-21 9:24 ` Mark Cave-Ayland 1 sibling, 1 reply; 9+ messages in thread From: Bernhard Beschow @ 2022-05-20 17:45 UTC (permalink / raw) To: qemu-devel Cc: qemu-trivial, Bernhard Beschow, Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost 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); 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), 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 ISADevice *mc146818_rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq); -- 2.36.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset 2022-05-20 17:45 ` [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset Bernhard Beschow @ 2022-05-21 9:24 ` Mark Cave-Ayland 2022-05-22 9:07 ` Bernhard Beschow 0 siblings, 1 reply; 9+ messages in thread From: Mark Cave-Ayland @ 2022-05-21 9:24 UTC (permalink / raw) To: Bernhard Beschow, qemu-devel Cc: qemu-trivial, Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset 2022-05-21 9:24 ` Mark Cave-Ayland @ 2022-05-22 9:07 ` Bernhard Beschow 2022-05-22 12:13 ` Mark Cave-Ayland 0 siblings, 1 reply; 9+ messages in thread From: Bernhard Beschow @ 2022-05-22 9:07 UTC (permalink / raw) To: Mark Cave-Ayland Cc: QEMU Developers, QEMU Trivial, Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost [-- Attachment #1: Type: text/plain, Size: 4694 bytes --] On Sat, May 21, 2022 at 11:24 AM Mark Cave-Ayland < mark.cave-ayland@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 <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. > [-- Attachment #2: Type: text/html, Size: 6662 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset 2022-05-22 9:07 ` Bernhard Beschow @ 2022-05-22 12:13 ` Mark Cave-Ayland 2022-05-22 12:16 ` Bernhard Beschow 0 siblings, 1 reply; 9+ messages in thread From: Mark Cave-Ayland @ 2022-05-22 12:13 UTC (permalink / raw) To: Bernhard Beschow Cc: QEMU Developers, QEMU Trivial, Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost On 22/05/2022 10:07, Bernhard Beschow wrote: > On Sat, May 21, 2022 at 11:24 AM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk > <mailto:mark.cave-ayland@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 <shentey@gmail.com <mailto: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. 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? > > 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 > <mailto:mark.cave-ayland@ilande.co.uk>> > > > ATB, > > Mark. ATB, Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset 2022-05-22 12:13 ` Mark Cave-Ayland @ 2022-05-22 12:16 ` Bernhard Beschow 0 siblings, 0 replies; 9+ messages in thread From: Bernhard Beschow @ 2022-05-22 12:16 UTC (permalink / raw) To: Mark Cave-Ayland Cc: QEMU Developers, QEMU Trivial, Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost Am 22. Mai 2022 12:13:48 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@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-ayland@ilande.co.uk <mailto:mark.cave-ayland@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 <shentey@gmail.com <mailto: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. > >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-ayland@ilande.co.uk >> <mailto:mark.cave-ayland@ilande.co.uk>> >> >> >> ATB, >> >> Mark. > > >ATB, > >Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-22 12:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-05-20 17:45 [PATCH 0/2] QOM improvements for rtc/mc146818rtc Bernhard Beschow 2022-05-20 17:45 ` [PATCH 1/2] hw/i386/microvm-dt: Determine mc146818rtc's IRQ number from QOM property Bernhard Beschow 2022-05-21 9:19 ` Mark Cave-Ayland 2022-05-22 8:56 ` Bernhard Beschow 2022-05-20 17:45 ` [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset Bernhard Beschow 2022-05-21 9:24 ` Mark Cave-Ayland 2022-05-22 9:07 ` Bernhard Beschow 2022-05-22 12:13 ` Mark Cave-Ayland 2022-05-22 12:16 ` Bernhard Beschow
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).