| Message ID | 20200403080502.8154-8-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: i386 tweaks | expand |
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); > } >
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
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 >
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
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 >
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
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 > >
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); }
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(-)