HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32329
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
ACPI: Clarify serial bus revision and specific revision
Serial bus revision [Byte 3] and serial bus specific revision [Byte 9] are not the same.
Change-Id: I366f62e6aa0e9c0dfbc1ec17adeebc42a0e777eb Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 8 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/32329/1
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index c57ba48..bdb864c 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -377,7 +377,7 @@ desc_length = acpi_device_write_zero_len();
/* Byte 3: Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_I2C_SERIAL_BUS_REVISION_ID);
/* Byte 4: Resource Source Index is Reserved */ acpigen_emit_byte(0); @@ -401,7 +401,7 @@ acpigen_emit_word(i2c->mode_10bit);
/* Byte 9: Type Specific Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_I2C_SPECIFIC_REVISION_ID);
/* Byte 10-11: I2C Type Data Length */ type_length = acpi_device_write_zero_len(); @@ -435,7 +435,7 @@ desc_length = acpi_device_write_zero_len();
/* Byte 3: Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_SPI_SERIAL_BUS_REVISION_ID);
/* Byte 4: Resource Source Index is Reserved */ acpigen_emit_byte(0); @@ -464,7 +464,7 @@ acpigen_emit_word(flags);
/* Byte 9: Type Specific Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_SPI_SPECIFIC_REVISION_ID);
/* Byte 10-11: SPI Type Data Length */ type_length = acpi_device_write_zero_len(); diff --git a/src/arch/x86/include/arch/acpi_device.h b/src/arch/x86/include/arch/acpi_device.h index d26f330..f940ea3 100644 --- a/src/arch/x86/include/arch/acpi_device.h +++ b/src/arch/x86/include/arch/acpi_device.h @@ -311,7 +311,10 @@
#define ACPI_SERIAL_BUS_TYPE_I2C 1 #define ACPI_SERIAL_BUS_TYPE_SPI 2 -#define ACPI_SERIAL_BUS_REVISION_ID 1 +#define ACPI_I2C_SERIAL_BUS_REVISION_ID 1 /* TODO: upgrade to 2 */ +#define ACPI_I2C_SPECIFIC_REVISION_ID 1 +#define ACPI_SPI_SERIAL_BUS_REVISION_ID 1 +#define ACPI_SPI_SPECIFIC_REVISION_ID 1
/* * ACPI I2C Bus
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32329 )
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/32329/1/src/arch/x86/include/arch/acpi_devic... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/#/c/32329/1/src/arch/x86/include/arch/acpi_devic... PS1, Line 315: SPECIFIC TYPE_REVISION_ID will be better?
https://review.coreboot.org/#/c/32329/1/src/arch/x86/include/arch/acpi_devic... PS1, Line 317: #define ACPI_SPI_SPECIFIC_REVISION_ID TYPE_REVISION_ID will be better?
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32329 )
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
Patch Set 1: Code-Review+2
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32329 )
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32329/1/src/arch/x86/include/arch/acpi_devic... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/#/c/32329/1/src/arch/x86/include/arch/acpi_devic... PS1, Line 317: #define ACPI_SPI_SPECIFIC_REVISION_ID
TYPE_REVISION_ID will be better?
Thank you for the review. "Type Specific Revision ID" is indeed used as Field Name, but this name is used for i2c, spi and uart. in the description/definition inside ACPI specification, they use "Indicates the revision of the I2C-specific Serial Bus". and "SPI-specific" ... to make a difference between them. so if you want we can use : ACPI_I2C_TYPE_SPECIFIC_REVISION_ID for i2c and ACPI_SPI_TYPE_SPECIFIC_REVISION_ID for spi.
Please let me know your thought.
Hello Patrick Rudolph, Lijian Zhao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32329
to look at the new patch set (#2).
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
ACPI: Clarify serial bus revision and specific revision
Serial bus revision [Byte 3] and serial bus specific revision [Byte 9] are not the same.
Change-Id: I366f62e6aa0e9c0dfbc1ec17adeebc42a0e777eb Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 10 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/32329/2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32329 )
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
Patch Set 2: Code-Review+2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32329 )
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32329/1/src/arch/x86/include/arch/acpi_devic... File src/arch/x86/include/arch/acpi_device.h:
https://review.coreboot.org/#/c/32329/1/src/arch/x86/include/arch/acpi_devic... PS1, Line 317: #define ACPI_SPI_SPECIFIC_REVISION_ID
Thank you for the review. […]
I think either one is fine
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32329 )
Change subject: ACPI: Clarify serial bus revision and specific revision ......................................................................
ACPI: Clarify serial bus revision and specific revision
Serial bus revision [Byte 3] and serial bus specific revision [Byte 9] are not the same.
Change-Id: I366f62e6aa0e9c0dfbc1ec17adeebc42a0e777eb Signed-off-by: Elyes HAOUAS ehaouas@noos.fr Reviewed-on: https://review.coreboot.org/c/coreboot/+/32329 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Lijian Zhao lijian.zhao@intel.com --- M src/arch/x86/acpi_device.c M src/arch/x86/include/arch/acpi_device.h 2 files changed, 10 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Lijian Zhao: Looks good to me, approved
diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c index c57ba48..5d8777f 100644 --- a/src/arch/x86/acpi_device.c +++ b/src/arch/x86/acpi_device.c @@ -377,7 +377,7 @@ desc_length = acpi_device_write_zero_len();
/* Byte 3: Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_I2C_SERIAL_BUS_REVISION_ID);
/* Byte 4: Resource Source Index is Reserved */ acpigen_emit_byte(0); @@ -401,7 +401,7 @@ acpigen_emit_word(i2c->mode_10bit);
/* Byte 9: Type Specific Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_I2C_TYPE_SPECIFIC_REVISION_ID);
/* Byte 10-11: I2C Type Data Length */ type_length = acpi_device_write_zero_len(); @@ -435,7 +435,7 @@ desc_length = acpi_device_write_zero_len();
/* Byte 3: Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_SPI_SERIAL_BUS_REVISION_ID);
/* Byte 4: Resource Source Index is Reserved */ acpigen_emit_byte(0); @@ -464,7 +464,7 @@ acpigen_emit_word(flags);
/* Byte 9: Type Specific Revision ID */ - acpigen_emit_byte(ACPI_SERIAL_BUS_REVISION_ID); + acpigen_emit_byte(ACPI_SPI_TYPE_SPECIFIC_REVISION_ID);
/* Byte 10-11: SPI Type Data Length */ type_length = acpi_device_write_zero_len(); diff --git a/src/arch/x86/include/arch/acpi_device.h b/src/arch/x86/include/arch/acpi_device.h index d26f330..3569546 100644 --- a/src/arch/x86/include/arch/acpi_device.h +++ b/src/arch/x86/include/arch/acpi_device.h @@ -309,9 +309,12 @@ * ACPI Descriptors for Serial Bus interfaces */
-#define ACPI_SERIAL_BUS_TYPE_I2C 1 -#define ACPI_SERIAL_BUS_TYPE_SPI 2 -#define ACPI_SERIAL_BUS_REVISION_ID 1 +#define ACPI_SERIAL_BUS_TYPE_I2C 1 +#define ACPI_SERIAL_BUS_TYPE_SPI 2 +#define ACPI_I2C_SERIAL_BUS_REVISION_ID 1 /* TODO: upgrade to 2 */ +#define ACPI_I2C_TYPE_SPECIFIC_REVISION_ID 1 +#define ACPI_SPI_SERIAL_BUS_REVISION_ID 1 +#define ACPI_SPI_TYPE_SPECIFIC_REVISION_ID 1
/* * ACPI I2C Bus
-
HAOUAS Elyes (Code Review) -
Lijian Zhao (Code Review)
-
Patrick Georgi (Code Review)