Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42971
to review the following change.
Change subject: mb/google/zork: Generate I2SM ACPI device at runtime ......................................................................
mb/google/zork: Generate I2SM ACPI device at runtime
This change moves the generation of I2SM ACPI device from static asl file to runtime generation by ACP device driver. dmic_select_gpio is set to match version 3+ of Trembyle and Dalboz schematics. In order to maintain backward compatibility, dmic_select_gpio is updated at runtime using variant_audio_update for board versions that are prior to version 3 of reference schematics.
The only difference from static generation is that the device I2SM is added under ACPD (i.e. ACP device) instead of CREC (Chrome EC device). It does not make any functional difference from the kernel perspective.
BUG=b:157603026 TEST=Verified that the following device gets generated in SSDT: Scope (_SB.PCI0.PBRA.ACPD) { Device (I2SM) { Name (_HID, "AMDI5682") // _HID: Hardware ID Name (_UID, One) // _UID: Unique ID Name (_DDN, "I2S machine driver") // _DDN: DOS Device Name Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x000D } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "dmic-gpios", Package (0x04) { _SB.PCI0.PBRA.ACPD.I2SM, Zero, Zero, Zero } } } }) } } Verified audio via speakers and mic input.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I5d1602c7f719eef9487ddea68e429d27408f9a76 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Commit-Queue: Furquan Shaikh furquan@chromium.org Tested-by: Furquan Shaikh furquan@chromium.org Reviewed-by: Aaron Durbin adurbin@google.com --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/Makefile.inc M src/mainboard/google/zork/variants/baseboard/devicetree.cb M src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/zork/variants/baseboard/ramstage_common.c 6 files changed, 42 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/42971/1
diff --git a/src/mainboard/google/zork/mainboard.c b/src/mainboard/google/zork/mainboard.c index 283f0e0..f295271 100644 --- a/src/mainboard/google/zork/mainboard.c +++ b/src/mainboard/google/zork/mainboard.c @@ -132,6 +132,12 @@ override_num_gpios); }
+static void mainboard_devtree_update(void) +{ + variant_audio_update(); + variant_devtree_update(); +} + static void mainboard_init(void *chip_info) { const struct sci_source *gpes; @@ -145,7 +151,7 @@ mainboard_configure_gpios();
/* Update DUT configuration */ - variant_devtree_update(); + mainboard_devtree_update();
/* * Some platforms use SCI not generated by a GPIO pin (event above 23). diff --git a/src/mainboard/google/zork/variants/baseboard/Makefile.inc b/src/mainboard/google/zork/variants/baseboard/Makefile.inc index 7e4ccae..f01af31 100644 --- a/src/mainboard/google/zork/variants/baseboard/Makefile.inc +++ b/src/mainboard/google/zork/variants/baseboard/Makefile.inc @@ -23,6 +23,7 @@ ramstage-$(CONFIG_BOARD_GOOGLE_BASEBOARD_DALBOZ) += fsps_baseboard_dalboz.c ramstage-y += helpers.c ramstage-y += tpm_tis.c +ramstage-y += ramstage_common.c
# Add OEM ID table ifeq ($(CONFIG_USE_OEM_BIN),y) diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree.cb b/src/mainboard/google/zork/variants/baseboard/devicetree.cb index 3cc311b..9324749 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree.cb @@ -16,6 +16,16 @@
register "acp_pin_cfg" = "I2S_PINS_I2S_TDM"
+ # DMIC select GPIO for ACP machine device + # This GPIO is used to select DMIC0 or DMIC1 by the kernel driver. It does not + # really have a polarity since low and high control the selection of DMIC and + # hence does not have an active polarity. + # Kernel driver does not use the polarity field and instead treats the GPIO + # selection as follows: + # Set low (0) = Select DMIC0 + # Set high (1) = Select DMIC1 + register "dmic_select_gpio" = "ACPI_GPIO_OUTPUT(GPIO_67)" + # Start : OPN Performance Configuration # (Configuratin that is common for all variants) # For the below fields, 0 indicates use SOC default diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl b/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl index ae5663e..b892570 100644 --- a/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl @@ -3,43 +3,3 @@ Scope (EC0.CREC) { #include <ec/google/chromeec/acpi/codec.asl> } - -/* machine driver */ -Device (I2SM) -{ - Name (_HID, "AMDI5682") - Name (_UID, 1) - Name (_DDN, "I2S machine Driver") - - Name (_CRS, ResourceTemplate () - { -#if CONFIG(BOARD_GOOGLE_BASEBOARD_DALBOZ) - /* DMIC select GPIO */ - GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, - IoRestrictionNone, "\_SB.GPIO", 0x00, - ResourceConsumer,,) { 6 } -#else - /* DMIC select GPIO */ - GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, - IoRestrictionNone, "\_SB.GPIO", 0x00, - ResourceConsumer,,) { 13 } -#endif - }) - /* Device-Specific Data */ - Name (_DSD, Package () - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package () - { - Package () - { - "dmic-gpio", Package () { ^I2SM, 0, 0, 0 } - } - } - - }) - Method (_STA) - { - Return (0xF) - } -} diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h index 4f8f225..41b8290 100644 --- a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h @@ -28,6 +28,8 @@ void variant_romstage_entry(void); /* Modify devictree settings during ramstage. */ void variant_devtree_update(void); +/* Update audio configuration in devicetree during ramstage. */ +void variant_audio_update(void); /* Configure PCIe power and reset lines as per variant sequencing requirements. */ void variant_pcie_power_reset_configure(void);
diff --git a/src/mainboard/google/zork/variants/baseboard/ramstage_common.c b/src/mainboard/google/zork/variants/baseboard/ramstage_common.c new file mode 100644 index 0000000..6372af6 --- /dev/null +++ b/src/mainboard/google/zork/variants/baseboard/ramstage_common.c @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <acpi/acpi_device.h> +#include <baseboard/variants.h> +#include <ec/google/chromeec/ec.h> +#include <soc/gpio.h> + +void variant_audio_update(void) +{ + struct soc_amd_picasso_config *cfg = config_of_soc(); + uint32_t board_version; + struct acpi_gpio *gpio = &cfg->dmic_select_gpio; + + if (!google_chromeec_cbi_get_board_version(&board_version) && + (board_version >= CONFIG_VARIANT_MIN_BOARD_ID_V3_SCHEMATICS)) + return; + + if (CONFIG(BOARD_GOOGLE_BASEBOARD_TREMBYLE)) + gpio->pins[0] = GPIO_13; + else + gpio->pins[0] = GPIO_6; +}
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42971 )
Change subject: mb/google/zork: Generate I2SM ACPI device at runtime ......................................................................
Patch Set 1: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42971 )
Change subject: mb/google/zork: Generate I2SM ACPI device at runtime ......................................................................
Patch Set 1: Code-Review+2
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Menzel, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42971
to look at the new patch set (#3).
Change subject: mb/google/zork: Generate I2SM ACPI device at runtime ......................................................................
mb/google/zork: Generate I2SM ACPI device at runtime
This change moves the generation of I2SM ACPI device from static asl file to runtime generation by ACP device driver. dmic_select_gpio is set to match version 3+ of Trembyle and Dalboz schematics. In order to maintain backward compatibility, dmic_select_gpio is updated at runtime using variant_audio_update for board versions that are prior to version 3 of reference schematics.
The only difference from static generation is that the device I2SM is added under ACPD (i.e. ACP device) instead of CREC (Chrome EC device). It does not make any functional difference from the kernel perspective.
BUG=b:157603026 TEST=Verified that the following device gets generated in SSDT: Scope (_SB.PCI0.PBRA.ACPD) { Device (I2SM) { Name (_HID, "AMDI5682") // _HID: Hardware ID Name (_UID, One) // _UID: Unique ID Name (_DDN, "I2S machine driver") // _DDN: DOS Device Name Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x000D } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "dmic-gpios", Package (0x04) { _SB.PCI0.PBRA.ACPD.I2SM, Zero, Zero, Zero } } } }) } } Verified audio via speakers and mic input.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I5d1602c7f719eef9487ddea68e429d27408f9a76 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Commit-Queue: Furquan Shaikh furquan@chromium.org Tested-by: Furquan Shaikh furquan@chromium.org Reviewed-by: Aaron Durbin adurbin@google.com --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/Makefile.inc M src/mainboard/google/zork/variants/baseboard/devicetree.cb M src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/zork/variants/baseboard/ramstage_common.c 6 files changed, 42 insertions(+), 41 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/42971/3
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42971 )
Change subject: mb/google/zork: Generate I2SM ACPI device at runtime ......................................................................
mb/google/zork: Generate I2SM ACPI device at runtime
This change moves the generation of I2SM ACPI device from static asl file to runtime generation by ACP device driver. dmic_select_gpio is set to match version 3+ of Trembyle and Dalboz schematics. In order to maintain backward compatibility, dmic_select_gpio is updated at runtime using variant_audio_update for board versions that are prior to version 3 of reference schematics.
The only difference from static generation is that the device I2SM is added under ACPD (i.e. ACP device) instead of CREC (Chrome EC device). It does not make any functional difference from the kernel perspective.
BUG=b:157603026 TEST=Verified that the following device gets generated in SSDT: Scope (_SB.PCI0.PBRA.ACPD) { Device (I2SM) { Name (_HID, "AMDI5682") // _HID: Hardware ID Name (_UID, One) // _UID: Unique ID Name (_DDN, "I2S machine driver") // _DDN: DOS Device Name Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings { GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\_SB.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x000D } }) Name (_DSD, Package (0x02) // _DSD: Device-Specific Data { ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, Package (0x01) { Package (0x02) { "dmic-gpios", Package (0x04) { _SB.PCI0.PBRA.ACPD.I2SM, Zero, Zero, Zero } } } }) } } Verified audio via speakers and mic input.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I5d1602c7f719eef9487ddea68e429d27408f9a76 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... Commit-Queue: Furquan Shaikh furquan@chromium.org Tested-by: Furquan Shaikh furquan@chromium.org Reviewed-by: Aaron Durbin adurbin@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42971 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/mainboard/google/zork/mainboard.c M src/mainboard/google/zork/variants/baseboard/Makefile.inc M src/mainboard/google/zork/variants/baseboard/devicetree.cb M src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl M src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/zork/variants/baseboard/ramstage_common.c 6 files changed, 42 insertions(+), 41 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Aaron Durbin: Looks good to me, approved
diff --git a/src/mainboard/google/zork/mainboard.c b/src/mainboard/google/zork/mainboard.c index 283f0e0..f295271 100644 --- a/src/mainboard/google/zork/mainboard.c +++ b/src/mainboard/google/zork/mainboard.c @@ -132,6 +132,12 @@ override_num_gpios); }
+static void mainboard_devtree_update(void) +{ + variant_audio_update(); + variant_devtree_update(); +} + static void mainboard_init(void *chip_info) { const struct sci_source *gpes; @@ -145,7 +151,7 @@ mainboard_configure_gpios();
/* Update DUT configuration */ - variant_devtree_update(); + mainboard_devtree_update();
/* * Some platforms use SCI not generated by a GPIO pin (event above 23). diff --git a/src/mainboard/google/zork/variants/baseboard/Makefile.inc b/src/mainboard/google/zork/variants/baseboard/Makefile.inc index c09fc7e..2b0ee3c 100644 --- a/src/mainboard/google/zork/variants/baseboard/Makefile.inc +++ b/src/mainboard/google/zork/variants/baseboard/Makefile.inc @@ -23,6 +23,7 @@ ramstage-$(CONFIG_BOARD_GOOGLE_BASEBOARD_DALBOZ) += fsps_baseboard_dalboz.c ramstage-y += helpers.c ramstage-y += tpm_tis.c +ramstage-y += ramstage_common.c
smm-$(CONFIG_BOARD_GOOGLE_BASEBOARD_TREMBYLE) += gpio_baseboard_trembyle.c smm-$(CONFIG_BOARD_GOOGLE_BASEBOARD_DALBOZ) += gpio_baseboard_dalboz.c diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree.cb b/src/mainboard/google/zork/variants/baseboard/devicetree.cb index 3cc311b..9324749 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree.cb @@ -16,6 +16,16 @@
register "acp_pin_cfg" = "I2S_PINS_I2S_TDM"
+ # DMIC select GPIO for ACP machine device + # This GPIO is used to select DMIC0 or DMIC1 by the kernel driver. It does not + # really have a polarity since low and high control the selection of DMIC and + # hence does not have an active polarity. + # Kernel driver does not use the polarity field and instead treats the GPIO + # selection as follows: + # Set low (0) = Select DMIC0 + # Set high (1) = Select DMIC1 + register "dmic_select_gpio" = "ACPI_GPIO_OUTPUT(GPIO_67)" + # Start : OPN Performance Configuration # (Configuratin that is common for all variants) # For the below fields, 0 indicates use SOC default diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl b/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl index ae5663e..b892570 100644 --- a/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/audio.asl @@ -3,43 +3,3 @@ Scope (EC0.CREC) { #include <ec/google/chromeec/acpi/codec.asl> } - -/* machine driver */ -Device (I2SM) -{ - Name (_HID, "AMDI5682") - Name (_UID, 1) - Name (_DDN, "I2S machine Driver") - - Name (_CRS, ResourceTemplate () - { -#if CONFIG(BOARD_GOOGLE_BASEBOARD_DALBOZ) - /* DMIC select GPIO */ - GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, - IoRestrictionNone, "\_SB.GPIO", 0x00, - ResourceConsumer,,) { 6 } -#else - /* DMIC select GPIO */ - GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, - IoRestrictionNone, "\_SB.GPIO", 0x00, - ResourceConsumer,,) { 13 } -#endif - }) - /* Device-Specific Data */ - Name (_DSD, Package () - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package () - { - Package () - { - "dmic-gpio", Package () { ^I2SM, 0, 0, 0 } - } - } - - }) - Method (_STA) - { - Return (0xF) - } -} diff --git a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h index 6c5b95b..c3986bc 100644 --- a/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/zork/variants/baseboard/include/baseboard/variants.h @@ -35,6 +35,8 @@ void variant_romstage_entry(void); /* Modify devictree settings during ramstage. */ void variant_devtree_update(void); +/* Update audio configuration in devicetree during ramstage. */ +void variant_audio_update(void); /* Configure PCIe power and reset lines as per variant sequencing requirements. */ void variant_pcie_power_reset_configure(void);
diff --git a/src/mainboard/google/zork/variants/baseboard/ramstage_common.c b/src/mainboard/google/zork/variants/baseboard/ramstage_common.c new file mode 100644 index 0000000..6372af6 --- /dev/null +++ b/src/mainboard/google/zork/variants/baseboard/ramstage_common.c @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <acpi/acpi_device.h> +#include <baseboard/variants.h> +#include <ec/google/chromeec/ec.h> +#include <soc/gpio.h> + +void variant_audio_update(void) +{ + struct soc_amd_picasso_config *cfg = config_of_soc(); + uint32_t board_version; + struct acpi_gpio *gpio = &cfg->dmic_select_gpio; + + if (!google_chromeec_cbi_get_board_version(&board_version) && + (board_version >= CONFIG_VARIANT_MIN_BOARD_ID_V3_SCHEMATICS)) + return; + + if (CONFIG(BOARD_GOOGLE_BASEBOARD_TREMBYLE)) + gpio->pins[0] = GPIO_13; + else + gpio->pins[0] = GPIO_6; +}
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)