Re: [Qemu-devel] [PATCH 4/7] hw/acpi/aml-build: Add aml_gpio_int() term

[Michael S. Tsirkin]

On Sun, May 31, 2015 at 08:13:37PM +0200, Michael S. Tsirkin wrote:
> On Tue, May 12, 2015 at 12:24:13PM +0800, address@hidden wrote:
> > From: Shannon Zhao <address@hidden>
> > 
> > Signed-off-by: Shannon Zhao <address@hidden>
> > Signed-off-by: Shannon Zhao <address@hidden>
> > ---
> >  hw/acpi/aml-build.c         | 60 
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/acpi/aml-build.h | 16 ++++++++++++
> >  2 files changed, 76 insertions(+)
> > 
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index 643e885..40d7fa0 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -525,6 +525,66 @@ Aml *aml_call4(const char *method, Aml *arg1, Aml 
> > *arg2, Aml *arg3, Aml *arg4)
> >  }
> >  
> >  /*
> > + * ACPI 5.0: 19.5.53
> > + * GpioInt(GPIO Interrupt Connection Resource Descriptor Macro)
> 
> GpioInt is an ASL thing, it has no place in QEMU as we
> don't use ASL.
> 
> What you should point at, and model API after, is
> 6.4.3.8.1 GPIO Connection Descriptor

You can make a wrapper for GpioInt if you like, but
it should be layered on top of GPIO Connection Descriptor.


> In fact, why not describe the layout with a struct?
> 
> That's much nicer than building it up byte by byte.
> Also, this way offsets will be self-documenting
> using sizeof.

Or at least, use build_append_int_noprefix for multibyte fields.

> > + */
> > +Aml *aml_gpio_int(AmlLevelAndEdge level_and_edge,
> > +                  AmlActiveHighAndLow high_and_low,
> > +                  AmlExclusiveAndShared exclusive_and_shared,
> > +                  AmlWakeCap wake_capable, AmlPinConfig pin_cfg,
> > +                  int32_t pin_num, const char *name)
> > +{
> > +    Aml *var = aml_alloc();
> > +    uint8_t flags = level_and_edge | (high_and_low << 1)
> > +                    | (exclusive_and_shared << 3) | (wake_capable << 4);
> > +
> > +    build_append_byte(var->buf, 0x8C); /* GpioInt Resource Descriptor */
> > +    /* Length */
> > +    build_append_byte(var->buf, 0x1B); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    build_append_byte(var->buf, 1); /* Revision ID */
> > +    /* GPIO Connection Type 0x00 = Interrupt Connection */
> > +    build_append_byte(var->buf, 0);
> > +    /* General Flags */
> > +    build_append_byte(var->buf, 1); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    /* Interrupt and IO Flags */
> > +    build_append_byte(var->buf, flags); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    /* Pin Configuration 0 = Default 1 = Pull-up 2 = Pull-down 3 = No Pull 
> > */
> > +    build_append_byte(var->buf, pin_cfg);
> > +    /* Output Drive Strength */
> > +    build_append_byte(var->buf, 0); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    /* Debounce timeout */
> > +    build_append_byte(var->buf, 0); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +
> > +    /* Pin Table Offset */
> > +    build_append_byte(var->buf, 0x17); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    /* Resource Source Index */
> > +    build_append_byte(var->buf, 0);
> > +    /* Resource Source Name */
> > +    build_append_byte(var->buf, 0x19); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    /* Vendor Data Offse */
> 
> Offset?
> 
> > +    build_append_byte(var->buf, 0x1E); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    /* Vendor Data Length */
> > +    build_append_byte(var->buf, 0); /* bits[7:0] */
> > +    build_append_byte(var->buf, 0); /* bits[15:8] */
> > +    /* Pin Numbe */
> 
> Number?
> 
> > +    build_append_byte(var->buf, pin_num & 0xff); /* bits[7:0] */
> > +    build_append_byte(var->buf, (pin_num >> 8) & 0xff); /* bits[15:8] */
> > +    /* Resource Source */
> > +    build_append_namestring(var->buf, "%s", name);
> > +    build_append_byte(var->buf, '\0');
> > +
> > +    return var;
> > +}
> > +
> > +/*
> >   * ACPI 1.0: 6.4.3.4 Memory32Fixed (Memory Resource Descriptor Macro)
> >   */
> >  Aml *aml_memory32_fixed(uint32_t addr, uint32_t size,
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index 39b44e4..b95a8e8 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -150,6 +150,17 @@ typedef enum {
> >      aml_wake_capable = 1,
> >  } AmlWakeCap;
> >  
> > +/*
> > + * ACPI 5.0: Table 6-189 GPIO Connection Descriptor Definition
> > + * _PPI field definition
> > + */
> > +typedef enum {
> > +    aml_default_config = 0,
> > +    aml_pull_up = 1,
> > +    aml_pull_down = 2,
> > +    aml_no_pull = 3,
> > +} AmlPinConfig;
> > +
> >  typedef
> >  struct AcpiBuildTables {
> >      GArray *table_data;
> > @@ -208,6 +219,11 @@ Aml *aml_call1(const char *method, Aml *arg1);
> >  Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
> >  Aml *aml_call3(const char *method, Aml *arg1, Aml *arg2, Aml *arg3);
> >  Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml 
> > *arg4);
> > +Aml *aml_gpio_int(AmlLevelAndEdge level_and_edge,
> > +                  AmlActiveHighAndLow high_and_low,
> > +                  AmlExclusiveAndShared exclusive_and_shared,
> > +                  AmlWakeCap wake_capable, AmlPinConfig pin_cfg,
> > +                  int32_t pin_num, const char *name);
> >  Aml *aml_memory32_fixed(uint32_t addr, uint32_t size,
> >                          AmlReadAndWrite read_and_write);
> >  Aml *aml_interrupt(AmlConsumerAndProducer con_and_pro,
> > -- 
> > 2.1.0
> >