diff mbox

[50/74] pc: acpi: move FDC0 device from DSDT to SSDT

Message ID 1449704528-289297-51-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Dec. 9, 2015, 11:41 p.m. UTC 
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c      | 38 ++++++++++++++++++++++++++++++++------
 hw/i386/acpi-dsdt-isa.dsl | 18 ------------------
 2 files changed, 32 insertions(+), 24 deletions(-)

Comments

Michael S. Tsirkin Dec. 19, 2015, 7:44 p.m. UTC | #1 
On Thu, Dec 10, 2015 at 12:41:44AM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c      | 38 ++++++++++++++++++++++++++++++++------
>  hw/i386/acpi-dsdt-isa.dsl | 18 ------------------
>  2 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e5ec6af..2f0f2e1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1173,17 +1173,34 @@ static void build_hpet_aml(Aml *table)
>  
>  static Aml *
>  build_eisa_device_aml(const char *name, const char *hid, Aml *crs,
> -                      bool have_sta)
> +                      bool have_sta, const char *present_field)

At this point, the number of arguments is getting ridiculous.
I think you should give it up, and write a function
per device type, even if there's a minor code duplication
as a result.

As a bonus, we will get reasonably readable function
names, e.g.  build_floppy_controller()

>  {
>      Aml *dev;
>      Aml *method;
> -    Aml *a_device_present = aml_int(0x0f);
>  
>      dev = aml_device("%s", name);
>      aml_append(dev, aml_name_decl("_HID", aml_eisaid(hid)));
>      if (have_sta) {
> +        Aml *if_ctx;
> +        Aml *else_ctx;
> +        Aml *a_zero = aml_int(0);
> +        Aml *a_is_present = aml_local(0);
> +        Aml *a_device_present = aml_int(0x0f);
> +        Aml *a_device_not_present = aml_int(0x00);
> +

What does a_ prefix mean here?

>          method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -        aml_append(method, aml_return(a_device_present));
> +        if (!present_field) {
> +            aml_append(method, aml_return(a_device_present));
> +        } else {
> +            aml_append(method,
> +                aml_store(aml_name("%s", present_field), a_is_present));
> +            if_ctx = aml_if(aml_equal(a_is_present, a_zero));
> +            aml_append(if_ctx, aml_return(a_device_not_present));
> +            aml_append(method, if_ctx);
> +            else_ctx = aml_else();
> +            aml_append(else_ctx, aml_return(a_device_present));
> +            aml_append(method, else_ctx);
> +        }
>          aml_append(dev, method);
>      }
>      aml_append(dev, aml_name_decl("_CRS", crs));
> @@ -1201,19 +1218,28 @@ static void build_isa_devices_aml(Aml *table)
>      aml_append(crs, aml_irq_no_flags(8));
>      aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
>      aml_append(scope, build_eisa_device_aml(
> -        "RTC", "PNP0B00", crs, false));
> +        "RTC", "PNP0B00", crs, false, NULL));
>  
>      crs = aml_resource_template();
>      aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
>      aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
>      aml_append(crs, aml_irq_no_flags(1));
>      aml_append(scope, build_eisa_device_aml(
> -        "KBD", "PNP0303", crs, true));
> +        "KBD", "PNP0303", crs, true, NULL));
>  
>      crs = aml_resource_template();
>      aml_append(crs, aml_irq_no_flags(12));
>      aml_append(scope, build_eisa_device_aml(
> -        "MOU", "PNP0F13", crs, true));
> +        "MOU", "PNP0F13", crs, true, NULL));
> +
> +    crs = aml_resource_template();
> +    aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
> +    aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
> +    aml_append(crs, aml_irq_no_flags(6));
> +    aml_append(crs,
> +        aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
> +    aml_append(scope, build_eisa_device_aml(
> +        "FDC0", "PNP0700", crs, true, "FDEN"));
>  
>      aml_append(table, scope);
>  }
> diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> index 8936271..64dd4ac 100644
> --- a/hw/i386/acpi-dsdt-isa.dsl
> +++ b/hw/i386/acpi-dsdt-isa.dsl
> @@ -16,24 +16,6 @@
>  /* Common legacy ISA style devices. */
>  Scope(\_SB.PCI0.ISA) {
>  
> -    Device(FDC0) {
> -        Name(_HID, EisaId("PNP0700"))
> -        Method(_STA, 0, NotSerialized) {
> -            Store(FDEN, Local0)
> -            If (LEqual(Local0, 0)) {
> -                Return (0x00)
> -            } Else {
> -                Return (0x0F)
> -            }
> -        }

All these guys have in common is the _STA method.
So write a function generating one, and be done with it.


> -        Name(_CRS, ResourceTemplate() {
> -            IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
> -            IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
> -            IRQNoFlags() { 6 }
> -            DMA(Compatibility, NotBusMaster, Transfer8) { 2 }
> -        })
> -    }
> -
>      Device(LPT) {
>          Name(_HID, EisaId("PNP0400"))
>          Method(_STA, 0, NotSerialized) {
> -- 
> 1.8.3.1
>
Igor Mammedov Dec. 21, 2015, 12:42 p.m. UTC | #2 
On Sat, 19 Dec 2015 21:44:55 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Dec 10, 2015 at 12:41:44AM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/acpi-build.c      | 38 ++++++++++++++++++++++++++++++++------
> >  hw/i386/acpi-dsdt-isa.dsl | 18 ------------------
> >  2 files changed, 32 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index e5ec6af..2f0f2e1 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1173,17 +1173,34 @@ static void build_hpet_aml(Aml *table)
> >  
> >  static Aml *
> >  build_eisa_device_aml(const char *name, const char *hid, Aml *crs,
> > -                      bool have_sta)
> > +                      bool have_sta, const char *present_field)
> 
> At this point, the number of arguments is getting ridiculous.
> I think you should give it up, and write a function
> per device type, even if there's a minor code duplication
> as a result.
> 
> As a bonus, we will get reasonably readable function
> names, e.g.  build_floppy_controller()
> 
> >  {
> >      Aml *dev;
> >      Aml *method;
> > -    Aml *a_device_present = aml_int(0x0f);
> >  
> >      dev = aml_device("%s", name);
> >      aml_append(dev, aml_name_decl("_HID", aml_eisaid(hid)));
> >      if (have_sta) {
> > +        Aml *if_ctx;
> > +        Aml *else_ctx;
> > +        Aml *a_zero = aml_int(0);
> > +        Aml *a_is_present = aml_local(0);
> > +        Aml *a_device_present = aml_int(0x0f);
> > +        Aml *a_device_not_present = aml_int(0x00);
> > +
> 
> What does a_ prefix mean here?
it's used to distinguish aml variables.

> 
> >          method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> > -        aml_append(method, aml_return(a_device_present));
> > +        if (!present_field) {
> > +            aml_append(method, aml_return(a_device_present));
> > +        } else {
> > +            aml_append(method,
> > +                aml_store(aml_name("%s", present_field), a_is_present));
> > +            if_ctx = aml_if(aml_equal(a_is_present, a_zero));
> > +            aml_append(if_ctx, aml_return(a_device_not_present));
> > +            aml_append(method, if_ctx);
> > +            else_ctx = aml_else();
> > +            aml_append(else_ctx, aml_return(a_device_present));
> > +            aml_append(method, else_ctx);
> > +        }
> >          aml_append(dev, method);
> >      }
> >      aml_append(dev, aml_name_decl("_CRS", crs));
> > @@ -1201,19 +1218,28 @@ static void build_isa_devices_aml(Aml *table)
> >      aml_append(crs, aml_irq_no_flags(8));
> >      aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
> >      aml_append(scope, build_eisa_device_aml(
> > -        "RTC", "PNP0B00", crs, false));
> > +        "RTC", "PNP0B00", crs, false, NULL));
> >  
> >      crs = aml_resource_template();
> >      aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
> >      aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
> >      aml_append(crs, aml_irq_no_flags(1));
> >      aml_append(scope, build_eisa_device_aml(
> > -        "KBD", "PNP0303", crs, true));
> > +        "KBD", "PNP0303", crs, true, NULL));
> >  
> >      crs = aml_resource_template();
> >      aml_append(crs, aml_irq_no_flags(12));
> >      aml_append(scope, build_eisa_device_aml(
> > -        "MOU", "PNP0F13", crs, true));
> > +        "MOU", "PNP0F13", crs, true, NULL));
> > +
> > +    crs = aml_resource_template();
> > +    aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
> > +    aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
> > +    aml_append(crs, aml_irq_no_flags(6));
> > +    aml_append(crs,
> > +        aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
> > +    aml_append(scope, build_eisa_device_aml(
> > +        "FDC0", "PNP0700", crs, true, "FDEN"));
> >  
> >      aml_append(table, scope);
> >  }
> > diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> > index 8936271..64dd4ac 100644
> > --- a/hw/i386/acpi-dsdt-isa.dsl
> > +++ b/hw/i386/acpi-dsdt-isa.dsl
> > @@ -16,24 +16,6 @@
> >  /* Common legacy ISA style devices. */
> >  Scope(\_SB.PCI0.ISA) {
> >  
> > -    Device(FDC0) {
> > -        Name(_HID, EisaId("PNP0700"))
> > -        Method(_STA, 0, NotSerialized) {
> > -            Store(FDEN, Local0)
> > -            If (LEqual(Local0, 0)) {
> > -                Return (0x00)
> > -            } Else {
> > -                Return (0x0F)
> > -            }
> > -        }
> 
> All these guys have in common is the _STA method.
> So write a function generating one, and be done with it.
I'll try to do so.

> 
> 
> > -        Name(_CRS, ResourceTemplate() {
> > -            IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
> > -            IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
> > -            IRQNoFlags() { 6 }
> > -            DMA(Compatibility, NotBusMaster, Transfer8) { 2 }
> > -        })
> > -    }
> > -
> >      Device(LPT) {
> >          Name(_HID, EisaId("PNP0400"))
> >          Method(_STA, 0, NotSerialized) {
> > -- 
> > 1.8.3.1
> >
Michael S. Tsirkin Dec. 21, 2015, 12:54 p.m. UTC | #3 
On Mon, Dec 21, 2015 at 01:42:39PM +0100, Igor Mammedov wrote:
> On Sat, 19 Dec 2015 21:44:55 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Dec 10, 2015 at 12:41:44AM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/acpi-build.c      | 38 ++++++++++++++++++++++++++++++++------
> > >  hw/i386/acpi-dsdt-isa.dsl | 18 ------------------
> > >  2 files changed, 32 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index e5ec6af..2f0f2e1 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1173,17 +1173,34 @@ static void build_hpet_aml(Aml *table)
> > >  
> > >  static Aml *
> > >  build_eisa_device_aml(const char *name, const char *hid, Aml *crs,
> > > -                      bool have_sta)
> > > +                      bool have_sta, const char *present_field)
> > 
> > At this point, the number of arguments is getting ridiculous.
> > I think you should give it up, and write a function
> > per device type, even if there's a minor code duplication
> > as a result.
> > 
> > As a bonus, we will get reasonably readable function
> > names, e.g.  build_floppy_controller()
> > 
> > >  {
> > >      Aml *dev;
> > >      Aml *method;
> > > -    Aml *a_device_present = aml_int(0x0f);
> > >  
> > >      dev = aml_device("%s", name);
> > >      aml_append(dev, aml_name_decl("_HID", aml_eisaid(hid)));
> > >      if (have_sta) {
> > > +        Aml *if_ctx;
> > > +        Aml *else_ctx;
> > > +        Aml *a_zero = aml_int(0);
> > > +        Aml *a_is_present = aml_local(0);
> > > +        Aml *a_device_present = aml_int(0x0f);
> > > +        Aml *a_device_not_present = aml_int(0x00);
> > > +
> > 
> > What does a_ prefix mean here?
> it's used to distinguish aml variables.


Ow, hungarian notation. Let's not go there please.

> > 
> > >          method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> > > -        aml_append(method, aml_return(a_device_present));
> > > +        if (!present_field) {
> > > +            aml_append(method, aml_return(a_device_present));
> > > +        } else {
> > > +            aml_append(method,
> > > +                aml_store(aml_name("%s", present_field), a_is_present));
> > > +            if_ctx = aml_if(aml_equal(a_is_present, a_zero));
> > > +            aml_append(if_ctx, aml_return(a_device_not_present));
> > > +            aml_append(method, if_ctx);
> > > +            else_ctx = aml_else();
> > > +            aml_append(else_ctx, aml_return(a_device_present));
> > > +            aml_append(method, else_ctx);
> > > +        }
> > >          aml_append(dev, method);
> > >      }
> > >      aml_append(dev, aml_name_decl("_CRS", crs));
> > > @@ -1201,19 +1218,28 @@ static void build_isa_devices_aml(Aml *table)
> > >      aml_append(crs, aml_irq_no_flags(8));
> > >      aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
> > >      aml_append(scope, build_eisa_device_aml(
> > > -        "RTC", "PNP0B00", crs, false));
> > > +        "RTC", "PNP0B00", crs, false, NULL));
> > >  
> > >      crs = aml_resource_template();
> > >      aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
> > >      aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
> > >      aml_append(crs, aml_irq_no_flags(1));
> > >      aml_append(scope, build_eisa_device_aml(
> > > -        "KBD", "PNP0303", crs, true));
> > > +        "KBD", "PNP0303", crs, true, NULL));
> > >  
> > >      crs = aml_resource_template();
> > >      aml_append(crs, aml_irq_no_flags(12));
> > >      aml_append(scope, build_eisa_device_aml(
> > > -        "MOU", "PNP0F13", crs, true));
> > > +        "MOU", "PNP0F13", crs, true, NULL));
> > > +
> > > +    crs = aml_resource_template();
> > > +    aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
> > > +    aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
> > > +    aml_append(crs, aml_irq_no_flags(6));
> > > +    aml_append(crs,
> > > +        aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
> > > +    aml_append(scope, build_eisa_device_aml(
> > > +        "FDC0", "PNP0700", crs, true, "FDEN"));
> > >  
> > >      aml_append(table, scope);
> > >  }
> > > diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
> > > index 8936271..64dd4ac 100644
> > > --- a/hw/i386/acpi-dsdt-isa.dsl
> > > +++ b/hw/i386/acpi-dsdt-isa.dsl
> > > @@ -16,24 +16,6 @@
> > >  /* Common legacy ISA style devices. */
> > >  Scope(\_SB.PCI0.ISA) {
> > >  
> > > -    Device(FDC0) {
> > > -        Name(_HID, EisaId("PNP0700"))
> > > -        Method(_STA, 0, NotSerialized) {
> > > -            Store(FDEN, Local0)
> > > -            If (LEqual(Local0, 0)) {
> > > -                Return (0x00)
> > > -            } Else {
> > > -                Return (0x0F)
> > > -            }
> > > -        }
> > 
> > All these guys have in common is the _STA method.
> > So write a function generating one, and be done with it.
> I'll try to do so.
> 
> > 
> > 
> > > -        Name(_CRS, ResourceTemplate() {
> > > -            IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
> > > -            IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
> > > -            IRQNoFlags() { 6 }
> > > -            DMA(Compatibility, NotBusMaster, Transfer8) { 2 }
> > > -        })
> > > -    }
> > > -
> > >      Device(LPT) {
> > >          Name(_HID, EisaId("PNP0400"))
> > >          Method(_STA, 0, NotSerialized) {
> > > -- 
> > > 1.8.3.1
> > >
diff mbox

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e5ec6af..2f0f2e1 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1173,17 +1173,34 @@  static void build_hpet_aml(Aml *table)
 
 static Aml *
 build_eisa_device_aml(const char *name, const char *hid, Aml *crs,
-                      bool have_sta)
+                      bool have_sta, const char *present_field)
 {
     Aml *dev;
     Aml *method;
-    Aml *a_device_present = aml_int(0x0f);
 
     dev = aml_device("%s", name);
     aml_append(dev, aml_name_decl("_HID", aml_eisaid(hid)));
     if (have_sta) {
+        Aml *if_ctx;
+        Aml *else_ctx;
+        Aml *a_zero = aml_int(0);
+        Aml *a_is_present = aml_local(0);
+        Aml *a_device_present = aml_int(0x0f);
+        Aml *a_device_not_present = aml_int(0x00);
+
         method = aml_method("_STA", 0, AML_NOTSERIALIZED);
-        aml_append(method, aml_return(a_device_present));
+        if (!present_field) {
+            aml_append(method, aml_return(a_device_present));
+        } else {
+            aml_append(method,
+                aml_store(aml_name("%s", present_field), a_is_present));
+            if_ctx = aml_if(aml_equal(a_is_present, a_zero));
+            aml_append(if_ctx, aml_return(a_device_not_present));
+            aml_append(method, if_ctx);
+            else_ctx = aml_else();
+            aml_append(else_ctx, aml_return(a_device_present));
+            aml_append(method, else_ctx);
+        }
         aml_append(dev, method);
     }
     aml_append(dev, aml_name_decl("_CRS", crs));
@@ -1201,19 +1218,28 @@  static void build_isa_devices_aml(Aml *table)
     aml_append(crs, aml_irq_no_flags(8));
     aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
     aml_append(scope, build_eisa_device_aml(
-        "RTC", "PNP0B00", crs, false));
+        "RTC", "PNP0B00", crs, false, NULL));
 
     crs = aml_resource_template();
     aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
     aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
     aml_append(crs, aml_irq_no_flags(1));
     aml_append(scope, build_eisa_device_aml(
-        "KBD", "PNP0303", crs, true));
+        "KBD", "PNP0303", crs, true, NULL));
 
     crs = aml_resource_template();
     aml_append(crs, aml_irq_no_flags(12));
     aml_append(scope, build_eisa_device_aml(
-        "MOU", "PNP0F13", crs, true));
+        "MOU", "PNP0F13", crs, true, NULL));
+
+    crs = aml_resource_template();
+    aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
+    aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
+    aml_append(crs, aml_irq_no_flags(6));
+    aml_append(crs,
+        aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
+    aml_append(scope, build_eisa_device_aml(
+        "FDC0", "PNP0700", crs, true, "FDEN"));
 
     aml_append(table, scope);
 }
diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl
index 8936271..64dd4ac 100644
--- a/hw/i386/acpi-dsdt-isa.dsl
+++ b/hw/i386/acpi-dsdt-isa.dsl
@@ -16,24 +16,6 @@ 
 /* Common legacy ISA style devices. */
 Scope(\_SB.PCI0.ISA) {
 
-    Device(FDC0) {
-        Name(_HID, EisaId("PNP0700"))
-        Method(_STA, 0, NotSerialized) {
-            Store(FDEN, Local0)
-            If (LEqual(Local0, 0)) {
-                Return (0x00)
-            } Else {
-                Return (0x0F)
-            }
-        }
-        Name(_CRS, ResourceTemplate() {
-            IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
-            IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
-            IRQNoFlags() { 6 }
-            DMA(Compatibility, NotBusMaster, Transfer8) { 2 }
-        })
-    }
-
     Device(LPT) {
         Name(_HID, EisaId("PNP0400"))
         Method(_STA, 0, NotSerialized) {