* [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
* [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 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 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 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
* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.