[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset
From: |
Bernhard Beschow |
Subject: |
Re: [PATCH 2/2] rtc/mc146818rtc: QOM'ify io_base offset |
Date: |
Sun, 22 May 2022 12:16:40 +0000 |
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.