Re: [Qemu-devel] [RFC PATCH 09/11] hw/acpi/acpi-build-utils: Add acpi_fixed_memory32() and acpi_extended_irq()

[Shannon Zhao]

Hi MST,

Thanks for your review :-)  Reply below.

On 2015/1/25 16:39, Michael S. Tsirkin wrote:
> On Sat, Jan 24, 2015 at 05:21:18PM +0800, Shannon Zhao wrote:
>> Add acpi_fixed_memory32() for describing device mmio region in resource 
>> template.
>> Add acpi_extended_irq() for describing device interrupt in resource template.
>> These can be used to generating DSDT table for ACPI on ARM.
>>
>> Signed-off-by: Shannon Zhao <address@hidden>
>> ---
>>  hw/acpi/acpi-build-utils.c         |   42 
>> ++++++++++++++++++++++++++++++++++++
>>  include/hw/acpi/acpi-build-utils.h |    2 +
>>  2 files changed, 44 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
>> index 59873e3..211c4d3 100644
>> --- a/hw/acpi/acpi-build-utils.c
>> +++ b/hw/acpi/acpi-build-utils.c
>> @@ -493,6 +493,48 @@ AcpiAml acpi_call4(const char *method, AcpiAml arg1, 
>> AcpiAml arg2,
>>  }
>>  
>>  /*
>> + * ACPI 5.1: 19.5.80 Memory32Fixed (Memory Resource Descriptor Macro)
> 
> Pls document the first spec which includes a feature, not the last one.
> 

Ok, thanks, will fix :-)

>> + *           6.4.2 Small Resource Data Type
> 
> So add API for small resource data type?
> 

Will fix.

>> + */
>> +AcpiAml acpi_fixed_memory32(uint64_t addr, uint64_t size, uint8_t rw_flag)
> 
> so name it memory32_fixed?
> 

Good idea

>> +{
>> +    AcpiAml var = aml_allocate_internal(0, NON_BLOCK);
>> +    build_append_byte(var.buf, 0x86); /* Extended irq descriptor */
>> +    build_append_byte(var.buf, 9);
>> +    build_append_byte(var.buf, 0);
>> +    build_append_byte(var.buf, rw_flag);
>> +    build_append_byte(var.buf, addr & 0xff);
>> +    build_append_byte(var.buf, (addr >> 8) & 0xff);
>> +    build_append_byte(var.buf, (addr >> 16) & 0xff);
>> +    build_append_byte(var.buf, (addr >> 24) & 0xff);
>> +
>> +    build_append_byte(var.buf, size & 0xff);
>> +    build_append_byte(var.buf, (size >> 8) & 0xff);
>> +    build_append_byte(var.buf, (size >> 16) & 0xff);
>> +    build_append_byte(var.buf, (size >> 24) & 0xff);
>> +    return var;
>> +}
>> +
>> +/*
>> + * ACPI 5.1: 19.5.61 Interrupt (Interrupt Resource Descriptor Macro)
>> + *           6.4.2 Small Resource Data Type
>> + */
>> +AcpiAml acpi_extended_irq(uint8_t irq_flags, int irq)
> 
> So acpi_interrupt?
> 

Ok

>> +{
>> +    AcpiAml var = aml_allocate_internal(0, NON_BLOCK);
>> +    build_append_byte(var.buf, 0x89); /* Extended irq descriptor */
>> +    build_append_byte(var.buf, 6);
>> +    build_append_byte(var.buf, 0);
>> +    build_append_byte(var.buf, irq_flags);
>> +    build_append_byte(var.buf, 0x01);
>> +    build_append_byte(var.buf, irq & 0xff);
>> +    build_append_byte(var.buf, (irq >> 8) & 0xff);
>> +    build_append_byte(var.buf, (irq >> 16) & 0xff);
>> +    build_append_byte(var.buf, (irq >> 24) & 0xff);
>> +    return var;
> 
> 
> Pls add comments to document opcode constants.
> 

Will add next version.

>> +}
>> +
>> +/*
>>   * ACPI 5.0: 19.5.62 IO (IO Resource Descriptor Macro)
>>   *           6.4.2 Small Resource Data Type
>>  */
>> diff --git a/include/hw/acpi/acpi-build-utils.h 
>> b/include/hw/acpi/acpi-build-utils.h
>> index d39b5b1..bfed546 100644
>> --- a/include/hw/acpi/acpi-build-utils.h
>> +++ b/include/hw/acpi/acpi-build-utils.h
>> @@ -115,6 +115,8 @@ AcpiAml acpi_call3(const char *method, AcpiAml arg1, 
>> AcpiAml arg2,
>>                     AcpiAml arg3);
>>  AcpiAml acpi_call4(const char *method, AcpiAml arg1, AcpiAml arg2,
>>                     AcpiAml arg3, AcpiAml arg4);
>> +AcpiAml acpi_fixed_memory32(uint64_t addr, uint64_t size, uint8_t rw_flag);
>> +AcpiAml acpi_extended_irq(uint8_t irq_flags, int irq);
>>  AcpiAml acpi_io(acpiIODecode dec, uint16_t min_base, uint16_t max_base,
>>                  uint8_t aln, uint8_t len);
>>  AcpiAml acpi_iqr_no_flags(uint8_t irq);
>> -- 
>> 1.7.1
>>
> 
> .
>