[v2,07/12] acpi: move aml builder code for rtc device
diff mbox series

Message ID 20200403080502.8154-8-kraxel@redhat.com
State New
Headers show
Series
  • acpi: i386 tweaks
Related show

Commit Message

Gerd Hoffmann April 3, 2020, 8:04 a.m. UTC
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/i386/acpi-build.c | 17 -----------------
 hw/rtc/mc146818rtc.c | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 17 deletions(-)

Comments

Igor Mammedov April 3, 2020, 10:09 a.m. UTC | #1
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));
> +    aml_append(crs, aml_irq_no_flags(8));
> +    aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));

since this is made a part of device, can we fetch io port values from
device instead of hard-codding values here?

> +
> +    dev = aml_device("RTC");
> +    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
> +    aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +    aml_append(scope, dev);
> +}
> +
>  static void rtc_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>      dc->realize = rtc_realizefn;
>      dc->reset = rtc_resetdev;
>      dc->vmsd = &vmstate_rtc;
> +    isa->build_aml = rtc_build_aml;
>      device_class_set_props(dc, mc146818rtc_properties);
>  }
>
Gerd Hoffmann April 6, 2020, 8:25 a.m. UTC | #2
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));
> > +    aml_append(crs, aml_irq_no_flags(8));
> > +    aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
> 
> 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.

cheers,
  Gerd
Igor Mammedov April 6, 2020, 12:17 p.m. UTC | #3
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));
> > > +    aml_append(crs, aml_irq_no_flags(8));
> > > +    aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));  
> > 
> > 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.
So we don't have a zoo of devices doing the same thing in multiple
ways.

> 
> cheers,
>   Gerd
>
Gerd Hoffmann April 7, 2020, 10:26 a.m. UTC | #4
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));
> > > > +    aml_append(crs, aml_irq_no_flags(8));
> > > > +    aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));  
> > > 
> > > 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 ...

> 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
Igor Mammedov April 8, 2020, 11:27 a.m. UTC | #5
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
>
Gerd Hoffmann April 8, 2020, 12:59 p.m. UTC | #6
Hi,

> > > > > > +    crs = aml_resource_template();
> > > > > > +    aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
> maybe replace magic 0x0070 with macro
>   RTC_BASE_ADDR

Yes, that sounds better.

> > > > > > +    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)

No idea, I've just moved around the code.

Good question though.  Also why this splitted in two ranges the first
place.  Looking at physical hardware (my workstation) I see this:

        Device (RTC)
        {
            Name (_HID, EisaId ("PNP0B00") /* AT Real-Time Clock */)  // _HID: Hardware ID
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x0070,             // Range Minimum
                    0x0070,             // Range Maximum
                    0x01,               // Alignment
                    0x08,               // Length
                    )
                IRQNoFlags ()
                    {8}
            })
        }

Clues anyone?

thanks,
  Gerd
Victor Lavaud via April 8, 2020, 8:28 p.m. UTC | #7
I'm curious why there's two ranges as well.

In our branch of QEMU, I've had to modify this RTC creation code to have only one range instead of two ranges.

Traditionally Macs have had one range for RTC and we have incompatibility with a two ranges.

If you could change it to one range without losing any compatibility, you'd get my thumbs up.

Cameron Esfahani
dirty@apple.com

"The cake is a lie."

Common wisdom



> On Apr 8, 2020, at 5:59 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>  Hi,
> 
>>>>>>> +    crs = aml_resource_template();
>>>>>>> +    aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
>> maybe replace magic 0x0070 with macro
>>  RTC_BASE_ADDR
> 
> Yes, that sounds better.
> 
>>>>>>> +    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)
> 
> No idea, I've just moved around the code.
> 
> Good question though.  Also why this splitted in two ranges the first
> place.  Looking at physical hardware (my workstation) I see this:
> 
>        Device (RTC)
>        {
>            Name (_HID, EisaId ("PNP0B00") /* AT Real-Time Clock */)  // _HID: Hardware ID
>            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
>            {
>                IO (Decode16,
>                    0x0070,             // Range Minimum
>                    0x0070,             // Range Maximum
>                    0x01,               // Alignment
>                    0x08,               // Length
>                    )
>                IRQNoFlags ()
>                    {8}
>            })
>        }
> 
> Clues anyone?
> 
> thanks,
>  Gerd
> 
>

Patch
diff mbox series

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 77fc9df74735..a5bc7764e611 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1131,22 +1131,6 @@  static Aml *build_fdc_device_aml(ISADevice *fdc)
     return dev;
 }
 
-static Aml *build_rtc_device_aml(void)
-{
-    Aml *dev;
-    Aml *crs;
-
-    dev = aml_device("RTC");
-    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
-    crs = aml_resource_template();
-    aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
-    aml_append(crs, aml_irq_no_flags(8));
-    aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
-    aml_append(dev, aml_name_decl("_CRS", crs));
-
-    return dev;
-}
-
 static Aml *build_kbd_device_aml(void)
 {
     Aml *dev;
@@ -1243,7 +1227,6 @@  static void build_isa_devices_aml(Aml *table)
     Aml *scope = aml_scope("_SB.PCI0.ISA");
     Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, &ambiguous);
 
-    aml_append(scope, build_rtc_device_aml());
     aml_append(scope, build_kbd_device_aml());
     aml_append(scope, build_mouse_device_aml());
     if (fdc) {
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index dc4269cc55cb..814263c01a90 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -27,6 +27,7 @@ 
 #include "qemu/cutils.h"
 #include "qemu/module.h"
 #include "qemu/bcd.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "qemu/timer.h"
@@ -1008,13 +1009,32 @@  static void rtc_resetdev(DeviceState *d)
     }
 }
 
+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));
+    aml_append(crs, aml_irq_no_flags(8));
+    aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
+
+    dev = aml_device("RTC");
+    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
+    aml_append(dev, aml_name_decl("_CRS", crs));
+
+    aml_append(scope, dev);
+}
+
 static void rtc_class_initfn(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
 
     dc->realize = rtc_realizefn;
     dc->reset = rtc_resetdev;
     dc->vmsd = &vmstate_rtc;
+    isa->build_aml = rtc_build_aml;
     device_class_set_props(dc, mc146818rtc_properties);
 }