Re: [PATCH v2 07/12] acpi: move aml builder code for rtc device

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 7 Apr 2020 12:26:58 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mon, Apr 06, 2020 at 02:17:05PM +0200, Igor Mammedov wrote:
> > On Mon, 6 Apr 2020 10:25:17 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> > > On Fri, Apr 03, 2020 at 12:09:21PM +0200, Igor Mammedov wrote:  
> > > > On Fri,  3 Apr 2020 10:04:57 +0200
> > > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > >     
> > > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > > > ---    
> > > > [...]    
> > > > > +static void rtc_build_aml(ISADevice *isadev, Aml *scope)
> > > > > +{
> > > > > +    Aml *dev;
> > > > > +    Aml *crs;
> > > > > +
> > > > > +    crs = aml_resource_template();
> > > > > +    aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
maybe replace magic 0x0070 with macro
  RTC_BASE_ADDR

and do the same in realize

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index dc4269cc55..a1f27f4883 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -908,7 +908,6 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 {
     ISADevice *isadev = ISA_DEVICE(dev);
     RTCState *s = MC146818_RTC(dev);
-    int base = 0x70;
 
     s->cmos_data[RTC_REG_A] = 0x26;
     s->cmos_data[RTC_REG_B] = 0x02;
@@ -951,7 +950,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, base);
+    isa_register_ioport(isadev, &s->io, RTC_BASE_ADDR);
 
     /* register rtc 0x70 port for coalesced_pio */
     memory_region_set_flush_coalesced(&s->io);
@@ -960,7 +959,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, base, 3);
+    qdev_set_legacy_instance_id(dev, RTC_BASE_ADDR, 3);
     qemu_register_reset(rtc_reset, s);
 
     object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);


> > > > > +    aml_append(crs, aml_irq_no_flags(8));
> > > > > +    aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));

one more comment, is this last io record correct?
(looking at realize it maps only 2 bytes at 0x70)
    
> > > > 
> > > > since this is made a part of device, can we fetch io port values from
> > > > device instead of hard-codding values here?    
> > > 
> > > No, the rtc device hasn't a configurable io port address.  
> > what I'm after is consistent code, so if we switch to taking
> > io_base/irq from ISA device, then do it everywhere.  
> 
> The patch series does it consistently where it makes sense.
> That IMHO isn't the case for the rtc.  It has a fixed address.
> You can't have multiple instances if it.  And because of that
> there isn't a variable in the device state struct where I could
> read the iobase from ...

ok

> 
> > So we don't have a zoo of devices doing the same thing in multiple
> > ways.  
> 
> It's two ways: hardcoded for devices which can't move and
> read-from-device for devices which can move.
> 
> cheers,
>   Gerd
>