diff mbox

[5/7] acpi: Add I2c serial bus CRS handling

Message ID 1462995966-1184-6-git-send-email-minyard@acm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Corey Minyard May 11, 2016, 7:46 p.m. UTC 
From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/acpi/aml-build.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h | 11 +++++++++++
 2 files changed, 53 insertions(+)

Comments

Michael S. Tsirkin May 12, 2016, 7:30 a.m. UTC | #1 
On Wed, May 11, 2016 at 02:46:04PM -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  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
Corey Minyard May 12, 2016, 1:26 p.m. UTC | #2 
On 05/12/2016 02:30 AM, Michael S. Tsirkin wrote:

Thanks for the comments, all fixed.

>>   /**
>>    * 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.

I'm not sure I follow here.  The address parameter is important, it's 
for the I2C address.  The top 8 bytes are reserved 0, but not the 
bottom.  The resource source parameter tells which I2C bus it's hooked to.

>
>> +                               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
Michael S. Tsirkin May 12, 2016, 1:33 p.m. UTC | #3 
On Thu, May 12, 2016 at 08:26:57AM -0500, Corey Minyard wrote:
> On 05/12/2016 02:30 AM, Michael S. Tsirkin wrote:
> 
> Thanks for the comments, all fixed.
> 
> >>  /**
> >>   * 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.
> 
> I'm not sure I follow here.  The address parameter is important, it's for
> the I2C address.  The top 8 bytes are reserved 0, but not the bottom.  The
> resource source parameter tells which I2C bus it's hooked to.

I mean flags and con_speed.

> >
> >>+                               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
diff mbox

Patch

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);
 }
+
+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 */
+    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);
+
+    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);
+    build_append_byte(var->buf, address & 0xff);
+    build_append_byte(var->buf, address >> 8);
+    g_array_append_vals(var->buf, resource_source, resource_source_len);
+
+    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)
+
 /**
  * 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,
+                               uint16_t address, const char *resource_source);
 
 /* Block AML object primitives */
 Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);