From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47706) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0l4z-0004Am-3f for qemu-devel@nongnu.org; Thu, 12 May 2016 03:30:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b0l4u-0007Ud-GG for qemu-devel@nongnu.org; Thu, 12 May 2016 03:30:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42883) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0l4u-0007UZ-8D for qemu-devel@nongnu.org; Thu, 12 May 2016 03:30:36 -0400 Date: Thu, 12 May 2016 10:30:32 +0300 From: "Michael S. Tsirkin" Message-ID: <20160512101937-mutt-send-email-mst@redhat.com> References: <1462995966-1184-1-git-send-email-minyard@acm.org> <1462995966-1184-6-git-send-email-minyard@acm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462995966-1184-6-git-send-email-minyard@acm.org> Subject: Re: [Qemu-devel] [PATCH 5/7] acpi: Add I2c serial bus CRS handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: minyard@acm.org Cc: Igor Mammedov , Paolo Bonzini , qemu-devel@nongnu.org, Corey Minyard On Wed, May 11, 2016 at 02:46:04PM -0500, minyard@acm.org wrote: > From: Corey Minyard > > Signed-off-by: Corey Minyard > --- > hw/acpi/aml-build.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > include/hw/acpi/aml-build.h | 11 +++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index ab89ca6..7a3874b 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -1563,3 +1563,45 @@ build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets, > build_header(linker, table_data, > (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id); > } > + Pls add comment with earliest spec revision that includes this. > +static Aml *aml_serial_bus_device(AmlSerialBusType type, uint8_t flags, > + uint16_t type_flags, > + uint8_t revid, uint16_t data_length, > + uint16_t resource_source_length) > +{ > + Aml *var = aml_alloc(); > + uint16_t length = data_length + resource_source_length + 9; > + > + build_append_byte(var->buf, 0x8e); /* Serial Bus Connection Descriptor */ > + build_append_byte(var->buf, length & 0xff); > + build_append_byte(var->buf, length >> 8); > + build_append_byte(var->buf, 1); /* Revision */ > + build_append_byte(var->buf, 0); /* Resource source index */ > + build_append_byte(var->buf, type); /* Serial bus type */ > + build_append_byte(var->buf, flags); /* Serial bus type */ Fix comments to match spec verbatim please. > + build_append_byte(var->buf, type_flags & 0xff); > + build_append_byte(var->buf, type_flags >> 8); > + build_append_byte(var->buf, revid); > + build_append_byte(var->buf, data_length & 0xff); > + build_append_byte(var->buf, data_length >> 8); > + Please use build_append_int_noprefix. > + return var; > +} > + > +Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed, > + uint16_t address, const char *resource_source) > +{ > + unsigned int resource_source_len = strlen(resource_source) + 1; > + Aml *var = aml_serial_bus_device(aml_serial_bus_type_i2c, flags, 0, 1, > + 6, resource_source_len); > + > + build_append_byte(var->buf, con_speed & 0xff); > + build_append_byte(var->buf, (con_speed >> 8) & 0xff); > + build_append_byte(var->buf, (con_speed >> 16) & 0xff); > + build_append_byte(var->buf, (con_speed >> 24) & 0xff); Please use build_append_int_noprefix. > + build_append_byte(var->buf, address & 0xff); > + build_append_byte(var->buf, address >> 8); Same. > + g_array_append_vals(var->buf, resource_source, resource_source_len); Is resource_source a name then? Then you should do aml_append(var, aml_name("%s", alias_object)); > + > + return var; > +} > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 2c994b3..1eb3ebd 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -206,6 +206,15 @@ struct AcpiBuildTables { > GArray *linker; > } AcpiBuildTables; > > +typedef enum { > + aml_serial_bus_type_i2c = 1, > + aml_serial_bus_type_spi = 2, > + aml_serial_bus_type_uart = 3 > +} AmlSerialBusType; > + > +#define AML_SERIAL_BUS_FLAG_MASTER_DEVICE (1 << 0) > +#define AML_SERIAL_BUS_FLAG_CONSUME_ONLY (1 << 1) > + Please drop enums and add numbers with comments matching text in ACPI spec in aml-build.c > /** > * init_aml_allocator: > * > @@ -327,6 +336,8 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed, > Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz, > uint8_t channel); > Aml *aml_sleep(uint64_t msec); > +Aml *aml_i2c_serial_bus_device(uint8_t flags, uint32_t con_speed, Drop these two parameters until they are actually useful. > + uint16_t address, const char *resource_source); > > /* Block AML object primitives */ > Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2); > -- > 2.7.4