Change in coreboot[master]: soc/amd/picasso: Add support for generating I2S machine device
Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42970
to review the following change.
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
soc/amd/picasso: Add support for generating I2S machine device
This change adds support in ACP device driver to generate I2S machine device(AMDI5682) in SSDT. It expects mainboard to provide chip config `dmic_select_gpio` that can be passed as `dmic-gpios` in _DSD for the device.
BUG=b:157603026 TEST=Verified that I2S machine device is correctly generated for trembyle.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I22ab53d7d68c6e042e467e598d688e360d28586f 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/soc/amd/picasso/acp.c M src/soc/amd/picasso/chip.h 2 files changed, 60 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42970/1
diff --git a/src/soc/amd/picasso/acp.c b/src/soc/amd/picasso/acp.c index 1e871e9..5bc1e27 100644 --- a/src/soc/amd/picasso/acp.c +++ b/src/soc/amd/picasso/acp.c @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> #include <console/console.h> #include <device/device.h> #include <device/pci.h> @@ -40,6 +42,60 @@ return "ACPD"; }
+#define AMD_I2S_ACPI_NAME "I2SM" +#define AMD_I2S_ACPI_HID "AMDI5682" +#define AMD_I2S_ACPI_DESC "I2S machine driver" + +static void acp_fill_i2s_machine_dev(const struct device *dev) +{ + const char *scope = acpi_device_path(dev); + const struct soc_amd_picasso_config *cfg = config_of_soc(); + const struct acpi_gpio *dmic_select_gpio = &cfg->dmic_select_gpio; + struct acpi_dp *dsd; + + if (dmic_select_gpio->pin_count == 0) + return; + + acpigen_write_scope(scope); /* Scope */ + acpigen_write_device(AMD_I2S_ACPI_NAME); /* Device */ + acpigen_write_name_string("_HID", AMD_I2S_ACPI_HID); + acpigen_write_name_integer("_UID", 1); + acpigen_write_name_string("_DDN", AMD_I2S_ACPI_DESC); + + acpigen_write_STA(ACPI_STATUS_DEVICE_ALL_ON); + + /* Resources */ + acpigen_write_name("_CRS"); + acpigen_write_resourcetemplate_header(); + acpi_device_write_gpio(dmic_select_gpio); + acpigen_write_resourcetemplate_footer(); + + dsd = acpi_dp_new_table("_DSD"); + /* + * 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 + */ + acpi_dp_add_gpio(dsd, "dmic-gpios", acpi_device_path_join(dev, AMD_I2S_ACPI_NAME), + 0, /* Index = 0 (There is a single GPIO entry in _CRS). */ + 0, /* Pin = 0 (There is a single pin in the GPIO resource). */ + 0);/* Active low = 0 (Kernel driver does not use active polarity). */ + acpi_dp_write(dsd); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ +} + +static void acp_fill_ssdt(const struct device *dev) +{ + acpi_device_write_pci_dev(dev); + acp_fill_i2s_machine_dev(dev); +} + static struct device_operations acp_ops = { .read_resources = pci_dev_read_resources, .set_resources = pci_dev_set_resources, @@ -47,7 +103,7 @@ .init = init, .ops_pci = &pci_dev_ops_pci, .acpi_name = acp_acpi_name, - .acpi_fill_ssdt = acpi_device_write_pci_dev, + .acpi_fill_ssdt = acp_fill_ssdt, };
static const struct pci_driver acp_driver __pci_driver = { diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index f5fbe0f..f11f800 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -46,6 +46,9 @@ uint8_t flags; } irq_override[16];
+ /* DMIC select GPIO for I2S machine driver AMDI5682 */ + struct acpi_gpio dmic_select_gpio; + /* Options for these are in src/arch/x86/include/acpi/acpi.h */ uint8_t fadt_pm_profile; uint16_t fadt_boot_arch;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42970 )
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42970/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42970/1//COMMIT_MSG@10 PS1, Line 10: device(AMDI5682) Please add a space before the (.
https://review.coreboot.org/c/coreboot/+/42970/1/src/soc/amd/picasso/acp.c File src/soc/amd/picasso/acp.c:
https://review.coreboot.org/c/coreboot/+/42970/1/src/soc/amd/picasso/acp.c@8... PS1, Line 86: 0);/* Active low = 0 (Kernel driver does not use active polarity). */ Please add a space in `;/*`.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42970 )
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42970/1/src/soc/amd/picasso/acp.c File src/soc/amd/picasso/acp.c:
https://review.coreboot.org/c/coreboot/+/42970/1/src/soc/amd/picasso/acp.c@8... PS1, Line 83: "dmic-gpios" This differs from the static asl. For posterity, acpi_find_gpio() eventually utilizes gpio_suffixes array of strings which is { "gpios", "gpio" }. So when acp3x-rt5682-max9836.c driver calls devm_gpiod_get(&pdev->dev, "dmic", GPIOD_OUT_LOW); that string is composed.
Hello build bot (Jenkins), Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42970
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
soc/amd/picasso: Add support for generating I2S machine device
This change adds support in ACP device driver to generate I2S machine device(AMDI5682) in SSDT. It expects mainboard to provide chip config `dmic_select_gpio` that can be passed as `dmic-gpios` in _DSD for the device.
BUG=b:157603026 TEST=Verified that I2S machine device is correctly generated for trembyle.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I22ab53d7d68c6e042e467e598d688e360d28586f 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/soc/amd/picasso/acp.c M src/soc/amd/picasso/chip.h 2 files changed, 60 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42970/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42970 )
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42970/1/src/soc/amd/picasso/acp.c File src/soc/amd/picasso/acp.c:
https://review.coreboot.org/c/coreboot/+/42970/1/src/soc/amd/picasso/acp.c@8... PS1, Line 83: "dmic-gpios"
This differs from the static asl. […]
ACK.
https://review.coreboot.org/c/coreboot/+/42970/1/src/soc/amd/picasso/acp.c@8... PS1, Line 86: 0);/* Active low = 0 (Kernel driver does not use active polarity). */
Please add a space in `;/*`.
Done
Hello build bot (Jenkins), Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42970
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
soc/amd/picasso: Add support for generating I2S machine device
This change adds support in ACP device driver to generate I2S machine device (AMDI5682) in SSDT. It expects mainboard to provide chip config `dmic_select_gpio` that can be passed as `dmic-gpios` in _DSD for the device.
BUG=b:157603026 TEST=Verified that I2S machine device is correctly generated for trembyle.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I22ab53d7d68c6e042e467e598d688e360d28586f 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/soc/amd/picasso/acp.c M src/soc/amd/picasso/chip.h 2 files changed, 60 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42970/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42970 )
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42970/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42970/1//COMMIT_MSG@10 PS1, Line 10: device(AMDI5682)
Please add a space before the (.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42970 )
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42970 )
Change subject: soc/amd/picasso: Add support for generating I2S machine device ......................................................................
soc/amd/picasso: Add support for generating I2S machine device
This change adds support in ACP device driver to generate I2S machine device (AMDI5682) in SSDT. It expects mainboard to provide chip config `dmic_select_gpio` that can be passed as `dmic-gpios` in _DSD for the device.
BUG=b:157603026 TEST=Verified that I2S machine device is correctly generated for trembyle.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I22ab53d7d68c6e042e467e598d688e360d28586f 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/+/42970 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/soc/amd/picasso/acp.c M src/soc/amd/picasso/chip.h 2 files changed, 60 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/soc/amd/picasso/acp.c b/src/soc/amd/picasso/acp.c index 1e871e9..3272acf 100644 --- a/src/soc/amd/picasso/acp.c +++ b/src/soc/amd/picasso/acp.c @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <acpi/acpi_device.h> +#include <acpi/acpigen.h> #include <console/console.h> #include <device/device.h> #include <device/pci.h> @@ -40,6 +42,60 @@ return "ACPD"; }
+#define AMD_I2S_ACPI_NAME "I2SM" +#define AMD_I2S_ACPI_HID "AMDI5682" +#define AMD_I2S_ACPI_DESC "I2S machine driver" + +static void acp_fill_i2s_machine_dev(const struct device *dev) +{ + const char *scope = acpi_device_path(dev); + const struct soc_amd_picasso_config *cfg = config_of_soc(); + const struct acpi_gpio *dmic_select_gpio = &cfg->dmic_select_gpio; + struct acpi_dp *dsd; + + if (dmic_select_gpio->pin_count == 0) + return; + + acpigen_write_scope(scope); /* Scope */ + acpigen_write_device(AMD_I2S_ACPI_NAME); /* Device */ + acpigen_write_name_string("_HID", AMD_I2S_ACPI_HID); + acpigen_write_name_integer("_UID", 1); + acpigen_write_name_string("_DDN", AMD_I2S_ACPI_DESC); + + acpigen_write_STA(ACPI_STATUS_DEVICE_ALL_ON); + + /* Resources */ + acpigen_write_name("_CRS"); + acpigen_write_resourcetemplate_header(); + acpi_device_write_gpio(dmic_select_gpio); + acpigen_write_resourcetemplate_footer(); + + dsd = acpi_dp_new_table("_DSD"); + /* + * 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 + */ + acpi_dp_add_gpio(dsd, "dmic-gpios", acpi_device_path_join(dev, AMD_I2S_ACPI_NAME), + 0, /* Index = 0 (There is a single GPIO entry in _CRS). */ + 0, /* Pin = 0 (There is a single pin in the GPIO resource). */ + 0); /* Active low = 0 (Kernel driver does not use active polarity). */ + acpi_dp_write(dsd); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ +} + +static void acp_fill_ssdt(const struct device *dev) +{ + acpi_device_write_pci_dev(dev); + acp_fill_i2s_machine_dev(dev); +} + static struct device_operations acp_ops = { .read_resources = pci_dev_read_resources, .set_resources = pci_dev_set_resources, @@ -47,7 +103,7 @@ .init = init, .ops_pci = &pci_dev_ops_pci, .acpi_name = acp_acpi_name, - .acpi_fill_ssdt = acpi_device_write_pci_dev, + .acpi_fill_ssdt = acp_fill_ssdt, };
static const struct pci_driver acp_driver __pci_driver = { diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h index f5fbe0f..f11f800 100644 --- a/src/soc/amd/picasso/chip.h +++ b/src/soc/amd/picasso/chip.h @@ -46,6 +46,9 @@ uint8_t flags; } irq_override[16];
+ /* DMIC select GPIO for I2S machine driver AMDI5682 */ + struct acpi_gpio dmic_select_gpio; + /* Options for these are in src/arch/x86/include/acpi/acpi.h */ uint8_t fadt_pm_profile; uint16_t fadt_boot_arch;
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)