Cheng-Yi Chiang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
Set calibration data in device property for RT1011
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 162 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36029/1
diff --git a/src/drivers/i2c/rt1011/Kconfig b/src/drivers/i2c/rt1011/Kconfig new file mode 100644 index 0000000..f795196 --- /dev/null +++ b/src/drivers/i2c/rt1011/Kconfig @@ -0,0 +1,4 @@ +config DRIVERS_I2C_RT1011 + bool + default n + depends on HAVE_ACPI_TABLES diff --git a/src/drivers/i2c/rt1011/Makefile.inc b/src/drivers/i2c/rt1011/Makefile.inc new file mode 100644 index 0000000..a8b8283 --- /dev/null +++ b/src/drivers/i2c/rt1011/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_I2C_RT1011) += rt1011.c diff --git a/src/drivers/i2c/rt1011/chip.h b/src/drivers/i2c/rt1011/chip.h new file mode 100644 index 0000000..39811a8 --- /dev/null +++ b/src/drivers/i2c/rt1011/chip.h @@ -0,0 +1,34 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * Realtek RT1011 audio codec devicetree bindings + */ + +#include <stdint.h> + +struct drivers_i2c_rt1011_config { + enum i2c_speed speed; /* Bus speed in Hz, default is I2C_SPEED_FAST */ + + const char *hid; /* ACPI _HID (required) */ + const char *name; /* ACPI Device Name */ + const char *desc; /* Device Description */ + unsigned int uid; /* ACPI _UID */ + + /* The VPD key of calibrated speaker resistance. */ + const char *r0_calib_key; + /* The VPD key of temperature during speaker calibration. */ + const char *temperature_calib_key; +}; diff --git a/src/drivers/i2c/rt1011/rt1011.c b/src/drivers/i2c/rt1011/rt1011.c new file mode 100644 index 0000000..b3bfda8 --- /dev/null +++ b/src/drivers/i2c/rt1011/rt1011.c @@ -0,0 +1,123 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/acpi.h> +#include <arch/acpi_device.h> +#include <arch/acpigen.h> +#include <console/console.h> +#include <device/i2c.h> +#include <device/device.h> +#include <device/path.h> +#include <stdint.h> +#include <dsm_calib.h> +#include "chip.h" + +#define RT1011_ACPI_NAME "RT11" + +#define RT1011_DP_INT(key, val) \ + acpi_dp_add_integer(dp, "realtek," key, (val)) + +static void rt1011_fill_ssdt(struct device *dev) +{ + struct drivers_i2c_rt1011_config *config = dev->chip_info; + const char *scope = acpi_device_scope(dev); + struct acpi_i2c i2c = { + .address = dev->path.i2c.device, + .mode_10bit = dev->path.i2c.mode_10bit, + .speed = config->speed ? : I2C_SPEED_FAST, + .resource = scope, + }; + struct acpi_dp *dp; + uint64_t r0_value, temp_value; + + if (!dev->enabled || !scope) + return; + + /* Device */ + acpigen_write_scope(scope); + acpigen_write_device(acpi_device_name(dev)); + acpigen_write_name_string("_HID", config->hid); + acpigen_write_name_integer("_UID", config->uid); + acpigen_write_name_string("_DDN", config->desc); + acpigen_write_STA(acpi_device_status(dev)); + + /* Resources */ + acpigen_write_name("_CRS"); + acpigen_write_resourcetemplate_header(); + acpi_device_write_i2c(&i2c); + + acpigen_write_resourcetemplate_footer(); + + /* Device Properties */ + printk(BIOS_ERR, "@@ device property\n"); + if (CONFIG(CHROMEOS_DSM_CALIB)) { + printk(BIOS_ERR, "@@ CONFIG_CHROMEOS_DSM_CALIB is on\n"); + if (get_dsm_calibration_from_key( + config->r0_calib_key, &r0_value) || + get_dsm_calibration_from_key( + config->temperature_calib_key, &temp_value)) { + printk(BIOS_ERR, "@@ cannot get dsm_calib\n"); + } else { + dp = acpi_dp_new_table("_DSD"); + RT1011_DP_INT("r0_calib", r0_value); + RT1011_DP_INT("temperature_calib", temp_value); + acpi_dp_write(dp); + printk(BIOS_ERR, "@@ success get dsm_calib\n"); + } + } + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s address 0%xh\n", acpi_device_path(dev), + dev->chip_ops->name, dev->path.i2c.device); +} + +static const char *rt1011_acpi_name(const struct device *dev) +{ + struct drivers_i2c_rt1011_config *config = dev->chip_info; + + if (config->name) + return config->name; + + return RT1011_ACPI_NAME; +} + +static struct device_operations rt1011_ops = { + .read_resources = DEVICE_NOOP, + .set_resources = DEVICE_NOOP, + .enable_resources = DEVICE_NOOP, + .acpi_name = rt1011_acpi_name, + .acpi_fill_ssdt_generator = rt1011_fill_ssdt, +}; + +static void rt1011_enable(struct device *dev) +{ + struct drivers_i2c_rt1011_config *config = dev->chip_info; + + if (!config) + return; + + dev->ops = &rt1011_ops; + + /* Name the device as per description provided in devicetree */ + if (config->desc) + dev->name = config->desc; +} + +struct chip_operations drivers_i2c_rt1011_ops = { + CHIP_NAME("Realtek RT1011 Codec") + .enable_dev = rt1011_enable +};
Hello Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36029
to look at the new patch set (#2).
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
Set calibration data in device property for RT1011 when config CHROMEOS_DSM_CALIB is turned on. Kernel rt1011 codec driver will use these device properties to setup codec accordingly.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 160 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36029/2
Hello Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36029
to look at the new patch set (#3).
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
Set calibration data in device property for RT1011 when config CHROMEOS_DSM_CALIB is turned on. Kernel rt1011 codec driver will use these device properties to setup codec accordingly.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 160 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36029/3
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 3: Code-Review+1
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36029/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36029/3//COMMIT_MSG@13 PS3, Line 13: Please add BUG, BRANCH, and TEST fields.
Hello Paul Fagerburg, Tim Wawrzynczak, Shelley Chen, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36029
to look at the new patch set (#4).
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
Set calibration data in device property for RT1011 when config CHROMEOS_DSM_CALIB is turned on. Kernel rt1011 codec driver will use these device properties to setup codec accordingly.
BUG=b:140397934 BRANCH=none TEST=On Helios, with patch series, check realtek,r0_calib and realtek,temperature_calib are available to rt1011 codec driver.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 160 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36029/4
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 5: Code-Review+1
Cheng-Yi Chiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36029/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36029/3//COMMIT_MSG@13 PS3, Line 13:
Please add BUG, BRANCH, and TEST fields.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 5:
(4 comments)
If you have not done yet, please run the newly created files through clang-format.
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@11 PS5, Line 11: setup Verb is spelled with a space: set up.
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@9 PS5, Line 9: Set calibration data in device property for RT1011 when config : CHROMEOS_DSM_CALIB is turned on. : Kernel rt1011 codec driver will use these device properties to setup : codec accordingly. Please use the full text width (75 characters) and separate paragraphs by a blank line.
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@13 PS5, Line 13: What datasheet (name, revision) did you use? Is it public?
https://review.coreboot.org/c/coreboot/+/36029/5/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/5/src/drivers/i2c/rt1011/rt10... PS5, Line 69: printk(BIOS_ERR, "cannot get dsm_calib from VPD\n"); Error messages should be more elaborate.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@8 PS5, Line 8: Oh, and please add the reasoning, why this driver is needed in coreboot anyway, and is not enough in the payload or OS.
Hello Paul Fagerburg, Duncan Laurie, Tim Wawrzynczak, Shelley Chen, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36029
to look at the new patch set (#6).
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
RT1011 is a smart amplifier. It needs to know speaker related parameters including speaker resistor value and temperature when the calibration is done in order to run Dynamic Speaker Management(DSM) algorithm on chip. The purpose of DSM is to protect speaker when the volume is large.
The calibration data of speaker is stored in VPD in factory. This driver is needed to read data from VPD and write to ACPI _DSD when config CHROMEOS_DSM_CALIB is turned on.
Kernel rt1011 codec driver will read these device properties to set up codec accordingly on boot.
The reason to prepare these parameters in coreboot is because kernel codec driver expects to read per-device parameters directly from device properties. Another benefit is that other OS can also take these parameters through ACPI _DSD table and take benefit of DSM on RT1011.
The kernel driver device properties of RT1011 are documented at linux/Documentation/devicetree/bindings/sound/rt1011.txt It is currently in ASoC maintainer's tree at https://kernel.googlesource.com/pub/scm/linux/kernel/git/broonie/sound/+/for... and hopefully should be merged to mainline kernel in the next merge window.
BUG=b:140397934 BRANCH=none TEST=On Helios, with patch series, check realtek,r0_calib and realtek,temperature_calib are available to rt1011 codec driver.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 159 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36029/6
Cheng-Yi Chiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 6:
(5 comments)
Hi Paul, Thanks for reviewing the patch. I have run clang-format on this patch. Also added more explanation to the commit message. Please let me know if you have other concern. Thanks!
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@8 PS5, Line 8:
Oh, and please add the reasoning, why this driver is needed in coreboot anyway, and is not enough in […]
Done
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@11 PS5, Line 11: setup
Verb is spelled with a space: set up.
Done
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@9 PS5, Line 9: Set calibration data in device property for RT1011 when config : CHROMEOS_DSM_CALIB is turned on. : Kernel rt1011 codec driver will use these device properties to setup : codec accordingly.
Please use the full text width (75 characters) and separate paragraphs by a blank line.
Done
https://review.coreboot.org/c/coreboot/+/36029/5//COMMIT_MSG@13 PS5, Line 13:
What datasheet (name, revision) did you use? Is it public?
Done
https://review.coreboot.org/c/coreboot/+/36029/5/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/5/src/drivers/i2c/rt1011/rt10... PS5, Line 69: printk(BIOS_ERR, "cannot get dsm_calib from VPD\n");
Error messages should be more elaborate.
Done
Cheng-Yi Chiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 6:
A gentle ping. Please let me know if I need to address some concern. Thanks!
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 6: Code-Review+1
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... File src/drivers/i2c/rt1011/chip.h:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... PS7, Line 25: /* ACPI _HID (required) */ Now that this isn't using the generic driver you might consider removing the hid setting and have the driver set it directly, since the kernel will only probe if the string is "10EC1011" the board doesn't actually have the freedom to set the HID. (and doesn't need to look up the magic value)
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 59: nit: empty line
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 119: CHIP_NAME("Realtek RT1011 Codec").enable_dev CHIP_NAME(x) is just ".name = x," so when expanded this looks a bit odd without a newline.
It compiles and everything, but is easier to follow when split on 2 lines.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... File src/drivers/i2c/rt1011/chip.h:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... PS7, Line 23: enum i2c_speed speed; /* Bus speed in Hz, default is I2C_SPEED_FAST */ Is this something that is configurable? If not, can we just assume I2C_SPEED_FAST in the driver and get rid of this member?
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 94: RT1011_ACPI_NAME If this same driver ends up getting used by multiple devices under the same parent, then there will be ACPI name collision.
Hello Paul Fagerburg, Duncan Laurie, Tim Wawrzynczak, Jimmy Cheng-Yi Chiang, Shelley Chen, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36029
to look at the new patch set (#8).
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
RT1011 is a smart amplifier. It needs to know speaker related parameters including speaker resistor value and temperature when the calibration is done in order to run Dynamic Speaker Management(DSM) algorithm on chip. The purpose of DSM is to protect speaker when the volume is large.
The calibration data of speaker is stored in VPD in factory. This driver is needed to read data from VPD and write to ACPI _DSD when config CHROMEOS_DSM_CALIB is turned on.
Kernel rt1011 codec driver will read these device properties to set up codec accordingly on boot.
The reason to prepare these parameters in coreboot is because kernel codec driver expects to read per-device parameters directly from device properties. Another benefit is that other OS can also take these parameters through ACPI _DSD table and take benefit of DSM on RT1011.
The kernel driver device properties of RT1011 are documented at linux/Documentation/devicetree/bindings/sound/rt1011.txt It is currently in ASoC maintainer's tree at https://kernel.googlesource.com/pub/scm/linux/kernel/git/broonie/sound/+/for... and hopefully should be merged to mainline kernel in the next merge window.
BUG=b:140397934 BRANCH=none TEST=On Helios, with patch series, check realtek,r0_calib and realtek,temperature_calib are available to rt1011 codec driver.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 160 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36029/8
Cheng-Yi Chiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 8:
(5 comments)
Thanks for reviewing!
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... File src/drivers/i2c/rt1011/chip.h:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... PS7, Line 23: enum i2c_speed speed; /* Bus speed in Hz, default is I2C_SPEED_FAST */
Is this something that is configurable? If not, can we just assume I2C_SPEED_FAST in the driver and […]
Done From the spec of ALC1011, the supported speed is 400K, that is, I2C_SPEED_FAST, so we can get rid of this member.
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/chip... PS7, Line 25: /* ACPI _HID (required) */
Now that this isn't using the generic driver you might consider removing the hid setting and have th […]
I see. Removed hid in config. Thanks for explanation.
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 59:
nit: empty line
Done
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 94: RT1011_ACPI_NAME
If this same driver ends up getting used by multiple devices under the same parent, then there will […]
Done. I used the same name as generic driver in case the name is not set from config.
https://review.coreboot.org/c/coreboot/+/36029/7/src/drivers/i2c/rt1011/rt10... PS7, Line 119: CHIP_NAME("Realtek RT1011 Codec").enable_dev
CHIP_NAME(x) is just ".name = x," so when expanded this looks a bit odd without a newline. […]
It was made by clang-format. I agree that splitting them to 2 lines looks better.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 9: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36029/9/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/9/src/drivers/i2c/rt1011/rt10... PS9, Line 95: name[4] = '\0'; nit: this isn't strictly necessary, snprintf will write a 0 to the end of the string/buffer, whichever comes first
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 9: Code-Review+1
(1 comment)
Thank you for the detailed explanation. I guess, ACPI is reality, but I still think, the OS driver should read it directly.
https://review.coreboot.org/c/coreboot/+/36029/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36029/9//COMMIT_MSG@11 PS9, Line 11: Management(DSM) Please add a space before the (.
Hello Paul Fagerburg, Paul Menzel, Tim Wawrzynczak, Jimmy Cheng-Yi Chiang, Duncan Laurie, Shelley Chen, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36029
to look at the new patch set (#10).
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
RT1011 is a smart amplifier. It needs to know speaker related parameters including speaker resistor value and temperature when the calibration is done in order to run Dynamic Speaker Management (DSM) algorithm on chip. The purpose of DSM is to protect speaker when the volume is large.
The calibration data of speaker is stored in VPD in factory. This driver is needed to read data from VPD and write to ACPI _DSD when config CHROMEOS_DSM_CALIB is turned on.
Kernel rt1011 codec driver will read these device properties to set up codec accordingly on boot.
The reason to prepare these parameters in coreboot is because kernel codec driver expects to read per-device parameters directly from device properties. Another benefit is that other OS can also take these parameters through ACPI _DSD table and take benefit of DSM on RT1011.
The kernel driver device properties of RT1011 are documented at linux/Documentation/devicetree/bindings/sound/rt1011.txt It is currently in ASoC maintainer's tree at https://kernel.googlesource.com/pub/scm/linux/kernel/git/broonie/sound/+/for... and hopefully should be merged to mainline kernel in the next merge window.
BUG=b:140397934 BRANCH=none TEST=On Helios, with patch series, check realtek,r0_calib and realtek,temperature_calib are available to rt1011 codec driver.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 159 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/36029/10
Cheng-Yi Chiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 11:
(2 comments)
Patch Set 9: Code-Review+1
(1 comment)
Thank you for the detailed explanation. I guess, ACPI is reality, but I still think, the OS driver should read it directly.
Hi Paul, Thanks for the understanding. I would really hope to merge this soon as project has its timeline. But I would still like to learn more about your concern. Are you generally against the idea of setting device property through coreboot ? Or is it because the information is from reading VPD? I am wondering because we have seen other cases of device driver in coreboot preparing _DSD, and other driver in coreboot reading VPD. So combining these steps to prepare device property from VPD seems to be a natural solution to me. Thanks!
https://review.coreboot.org/c/coreboot/+/36029/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36029/9//COMMIT_MSG@11 PS9, Line 11: Management(DSM)
Please add a space before the (.
Done
https://review.coreboot.org/c/coreboot/+/36029/9/src/drivers/i2c/rt1011/rt10... File src/drivers/i2c/rt1011/rt1011.c:
https://review.coreboot.org/c/coreboot/+/36029/9/src/drivers/i2c/rt1011/rt10... PS9, Line 95: name[4] = '\0';
nit: this isn't strictly necessary, snprintf will write a 0 to the end of the string/buffer, whichev […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
Patch Set 11: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36029 )
Change subject: drivers/i2c/rt1011: Add a driver for RT1011 ......................................................................
drivers/i2c/rt1011: Add a driver for RT1011
RT1011 is a smart amplifier. It needs to know speaker related parameters including speaker resistor value and temperature when the calibration is done in order to run Dynamic Speaker Management (DSM) algorithm on chip. The purpose of DSM is to protect speaker when the volume is large.
The calibration data of speaker is stored in VPD in factory. This driver is needed to read data from VPD and write to ACPI _DSD when config CHROMEOS_DSM_CALIB is turned on.
Kernel rt1011 codec driver will read these device properties to set up codec accordingly on boot.
The reason to prepare these parameters in coreboot is because kernel codec driver expects to read per-device parameters directly from device properties. Another benefit is that other OS can also take these parameters through ACPI _DSD table and take benefit of DSM on RT1011.
The kernel driver device properties of RT1011 are documented at linux/Documentation/devicetree/bindings/sound/rt1011.txt It is currently in ASoC maintainer's tree at https://kernel.googlesource.com/pub/scm/linux/kernel/git/broonie/sound/+/for... and hopefully should be merged to mainline kernel in the next merge window.
BUG=b:140397934 BRANCH=none TEST=On Helios, with patch series, check realtek,r0_calib and realtek,temperature_calib are available to rt1011 codec driver.
Signed-off-by: Cheng-Yi Chiang cychiang@chromium.org Change-Id: I9550b9890ce2cae787f4f17779a5ade77f619171 Reviewed-on: https://review.coreboot.org/c/coreboot/+/36029 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- A src/drivers/i2c/rt1011/Kconfig A src/drivers/i2c/rt1011/Makefile.inc A src/drivers/i2c/rt1011/chip.h A src/drivers/i2c/rt1011/rt1011.c 4 files changed, 159 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/i2c/rt1011/Kconfig b/src/drivers/i2c/rt1011/Kconfig new file mode 100644 index 0000000..f795196 --- /dev/null +++ b/src/drivers/i2c/rt1011/Kconfig @@ -0,0 +1,4 @@ +config DRIVERS_I2C_RT1011 + bool + default n + depends on HAVE_ACPI_TABLES diff --git a/src/drivers/i2c/rt1011/Makefile.inc b/src/drivers/i2c/rt1011/Makefile.inc new file mode 100644 index 0000000..a8b8283 --- /dev/null +++ b/src/drivers/i2c/rt1011/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_I2C_RT1011) += rt1011.c diff --git a/src/drivers/i2c/rt1011/chip.h b/src/drivers/i2c/rt1011/chip.h new file mode 100644 index 0000000..6bbddac --- /dev/null +++ b/src/drivers/i2c/rt1011/chip.h @@ -0,0 +1,31 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* + * Realtek RT1011 audio codec devicetree bindings + */ + +#include <stdint.h> + +struct drivers_i2c_rt1011_config { + const char *name; /* ACPI Device Name */ + const char *desc; /* Device Description */ + unsigned int uid; /* ACPI _UID */ + + /* The VPD key of calibrated speaker resistance. */ + const char *r0_calib_key; + /* The VPD key of temperature during speaker calibration. */ + const char *temperature_calib_key; +}; diff --git a/src/drivers/i2c/rt1011/rt1011.c b/src/drivers/i2c/rt1011/rt1011.c new file mode 100644 index 0000000..792992e --- /dev/null +++ b/src/drivers/i2c/rt1011/rt1011.c @@ -0,0 +1,123 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/acpi.h> +#include <arch/acpi_device.h> +#include <arch/acpigen.h> +#include <console/console.h> +#include <device/i2c.h> +#include <device/device.h> +#include <device/path.h> +#include <stdint.h> +#include <vendorcode/google/chromeos/chromeos.h> +#include "chip.h" + +#define RT1011_ACPI_HID "10EC1011" + +#define RT1011_DP_INT(key, val) acpi_dp_add_integer(dp, "realtek," key, (val)) + +static void rt1011_fill_ssdt(struct device *dev) +{ + struct drivers_i2c_rt1011_config *config = dev->chip_info; + const char *scope = acpi_device_scope(dev); + struct acpi_i2c i2c = { + .address = dev->path.i2c.device, + .mode_10bit = dev->path.i2c.mode_10bit, + .speed = I2C_SPEED_FAST, + .resource = scope, + }; + struct acpi_dp *dp; + uint64_t r0_value, temp_value; + + if (!dev->enabled || !scope) + return; + + /* Device */ + acpigen_write_scope(scope); + acpigen_write_device(acpi_device_name(dev)); + acpigen_write_name_string("_HID", RT1011_ACPI_HID); + acpigen_write_name_integer("_UID", config->uid); + acpigen_write_name_string("_DDN", config->desc); + acpigen_write_STA(acpi_device_status(dev)); + + /* Resources */ + acpigen_write_name("_CRS"); + acpigen_write_resourcetemplate_header(); + acpi_device_write_i2c(&i2c); + acpigen_write_resourcetemplate_footer(); + + /* Device Properties */ + if (CONFIG(CHROMEOS_DSM_CALIB)) { + if (get_dsm_calibration_from_key(config->r0_calib_key, &r0_value) + || get_dsm_calibration_from_key(config->temperature_calib_key, + &temp_value)) { + printk(BIOS_ERR, + "Failed to get dsm_calib parameters from VPD" + " with key %s and %s\n", + config->r0_calib_key, config->temperature_calib_key); + } else { + dp = acpi_dp_new_table("_DSD"); + RT1011_DP_INT("r0_calib", r0_value); + RT1011_DP_INT("temperature_calib", temp_value); + acpi_dp_write(dp); + printk(BIOS_INFO, "set dsm_calib properties\n"); + } + } + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: %s address 0%xh\n", acpi_device_path(dev), dev->chip_ops->name, + dev->path.i2c.device); +} + +static const char *rt1011_acpi_name(const struct device *dev) +{ + struct drivers_i2c_rt1011_config *config = dev->chip_info; + static char name[5]; + + if (config->name) + return config->name; + + snprintf(name, sizeof(name), "D%03.3X", dev->path.i2c.device); + return name; +} + +static struct device_operations rt1011_ops = { + .read_resources = DEVICE_NOOP, + .set_resources = DEVICE_NOOP, + .enable_resources = DEVICE_NOOP, + .acpi_name = rt1011_acpi_name, + .acpi_fill_ssdt_generator = rt1011_fill_ssdt, +}; + +static void rt1011_enable(struct device *dev) +{ + struct drivers_i2c_rt1011_config *config = dev->chip_info; + + if (!config) + return; + + dev->ops = &rt1011_ops; + + /* Name the device as per description provided in devicetree */ + if (config->desc) + dev->name = config->desc; +} + +struct chip_operations drivers_i2c_rt1011_ops = { + CHIP_NAME("Realtek RT1011 Codec") + .enable_dev = rt1011_enable +};
-
Cheng-Yi Chiang (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)
-
Shelley Chen (Code Review)
-
Tim Wawrzynczak (Code Review)