Re: [Qemu-devel] [PATCH v4 2/8] ARM: ACPI: Add GPIO controller in ACPI DSDT table

[Shannon Zhao <zhaoshenglong@huawei.com>]

Hi,

On 2015/12/11 4:13, Igor Mammedov wrote:
> On Thu, 10 Dec 2015 13:16:22 -0600
> Wei Huang <wei@redhat.com> wrote:
> 
>> > 
>> > 
>> > On 12/10/2015 07:22 AM, Igor Mammedov wrote:
>>> > > On Mon, 7 Dec 2015 15:39:46 +0800
>>> > > Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>>> > > 
>>>> > >> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>> > >>
>>>> > >> Add GPIO controller in ACPI DSDT table. It can be used for gpio
>>>> > >> event.
>>>> > >>
>>>> > >> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>>>> > >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>> > >> Tested-by: Wei Huang <wei@redhat.com>
>>>> > >> ---
>>>> > >>  hw/arm/virt-acpi-build.c | 19 +++++++++++++++++++
>>>> > >>  1 file changed, 19 insertions(+)
>>>> > >>
>>>> > >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> > >> index 3c2c5d6..bf6b934 100644
>>>> > >> --- a/hw/arm/virt-acpi-build.c
>>>> > >> +++ b/hw/arm/virt-acpi-build.c
>>>> > >> @@ -323,6 +323,23 @@ static void acpi_dsdt_add_pci(Aml *scope,
>>>> > >> const MemMapEntry *memmap, int irq, aml_append(scope, dev);
>>>> > >>  }
>>>> > >>  
>>>> > >> +static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry
>>>> > >> *gpio_memmap,
>>>> > >> +                                           int gpio_irq)
>>> > > s/int/uint32_t/
>>> > > 
>>>> > >> +{
>>>> > >> +    Aml *dev = aml_device("GPO0");
>>>> > >> +    aml_append(dev, aml_name_decl("_HID",
>>>> > >> aml_string("ARMH0061")));
>>> > > What spec "ARMH0061" comes from?
>>> > > Probably it should be mentioned in comment.
>> > 
>> > I tried to find the official source for this definition several weeks
>> > ago, but failed. Later, in private, Al Stone confirmed with me that
>> > ARMH0061 is the right one. Maybe ARM or Linaro should push this
>> > definition into ACPI spec or somewhere else in a formal way.
> I've also tried and only saw it on lkml in mentioned patchset,
> which wasn't warmly met.
> Given that and lack of spec where it's allocated perhaps we should wait
> on it till ARM puts in in some their spec formally.
> 
We discussed this at previous patchset with Peter Maydell and Al Stone.
I think ARM should publish a list of ACPI ID for its own devices. Al
also confirmed the _HID of PL061 is ARMH0061.

>> > 
>>> > > 
>>>> > >> +    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
>>>> > >> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>>>> > >> +
>>>> > >> +    Aml *crs = aml_resource_template();
>>>> > >> +    aml_append(crs, aml_memory32_fixed(gpio_memmap->base,
>>>> > >> gpio_memmap->size,
>>>> > >> +                                       AML_READ_WRITE));
>>>> > >> +    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL,
>>>> > >> AML_ACTIVE_HIGH,
>>>> > >> +                                  AML_EXCLUSIVE, gpio_irq));
>>> > > that conflicts with
>>> > > https://github.com/imammedo/qemu/commit/acb34e533bc31fdf3eb6230c93654b0b0ae4e76e
>>> > > perhaps you could include it in your series and redo this hunk
>>> > > to take array instead of int.
>>> > > 
sure

>>> > > 
>>>> > >> +    aml_append(dev, aml_name_decl("_CRS", crs));
>>>> > >> +    aml_append(scope, dev);
>>>> > >> +}
>>>> > >> +
>>>> > >>  /* RSDP */
>>>> > >>  static GArray *
>>>> > >>  build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>>>> > >> @@ -539,6 +556,8 @@ build_dsdt(GArray *table_data, GArray *linker,
>>>> > >> VirtGuestInfo *guest_info) (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
>>>> > >> NUM_VIRTIO_TRANSPORTS); acpi_dsdt_add_pci(scope, memmap,
>>>> > >> (irqmap[VIRT_PCIE] + ARM_SPI_BASE), guest_info->use_highmem);
>>>> > >> +    acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>>>> > >> +                       (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
>>>> > >>  
>>>> > >>      aml_append(dsdt, scope);
>>>> > >>  
>>> > > 

-- 
Shannon