| Message ID | 20200906154340.v3.39.I1a9174fd9af26d0891e4ccfac247c7359b28b23e@changeid |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | dm: Add programatic generation of ACPI tables (part D) | expand |
On Sun, Sep 06, 2020 at 03:43:47PM -0600, Simon Glass wrote: > Generate ACPI information for this device so that Linux can use it > correctly. > + ret = acpi_device_write_interrupt_or_gpio(ctx, (struct udevice *)dev, > + "ready-gpios"); > + if (ret < 0) > + return log_msg_ret("irq_gpio", ret); I looked a bit closer at the acpi_table.c and would like to emphasize on lessons learn from BIOS mistakes found in the wild with GPIOs. Because GPIO resources are quite badly described in ACPI (it seems MS failed to deliver GPIO abstraction to ACPI and to Windows API), there are some corner cases, in order to mitigate which we need to consider the following to avoid potential glitches and misconfiguration: - GpioIo() doesn't have any means of Active Low / High setting, the _DSD must be provided to mitigate this. - GpioIo() doesn't properly communicate the initial state of the output pin, thus Linux assumes the simple rule: Pull Bias Polarity Requested... Implicit x AS IS (assumed firmware configured for us) Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low), assuming non-active (Polarity = !Pull Bias) Down Low as low, assuming active Down High as high, assuming non-active Up Low as low, assuming non-active Up High as high, assuming active Hopefully this helps (and maybe can be added to some documentation). P.S. Why I2cSerialBus() and not I2cSerialBusV2() ?
On Mon, Sep 21, 2020 at 02:50:56PM +0300, Andy Shevchenko wrote: > On Sun, Sep 06, 2020 at 03:43:47PM -0600, Simon Glass wrote: > > Generate ACPI information for this device so that Linux can use it > > correctly. > > > + ret = acpi_device_write_interrupt_or_gpio(ctx, (struct udevice *)dev, > > + "ready-gpios"); > > + if (ret < 0) > > + return log_msg_ret("irq_gpio", ret); > > I looked a bit closer at the acpi_table.c and would like to emphasize on > lessons learn from BIOS mistakes found in the wild with GPIOs. > > Because GPIO resources are quite badly described in ACPI (it seems MS failed to > deliver GPIO abstraction to ACPI and to Windows API), there are some corner > cases, in order to mitigate which we need to consider the following to avoid > potential glitches and misconfiguration: > > - GpioIo() doesn't have any means of Active Low / High setting, the _DSD must > be provided to mitigate this. > > - GpioIo() doesn't properly communicate the initial state of the output pin, > thus Linux assumes the simple rule: > > Pull Bias Polarity Requested... > > Implicit x AS IS (assumed firmware configured for us) > Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low), > assuming non-active (Polarity = !Pull Bias) > > Down Low as low, assuming active > Down High as high, assuming non-active > Up Low as low, assuming non-active > Up High as high, assuming active > > Hopefully this helps (and maybe can be added to some documentation). > > P.S. Why I2cSerialBus() and not I2cSerialBusV2() ? Forgot one important aspect. GpioIo() can be used as interrupt and in this case the IoRestriction mustn't be OutputOnly. It also requires active_low flag from _DSD in cases where it's needed (better to always provide than rely on above assumption made on OS level).
Hi Simon, On Mon, Sep 21, 2020 at 7:51 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sun, Sep 06, 2020 at 03:43:47PM -0600, Simon Glass wrote: > > Generate ACPI information for this device so that Linux can use it > > correctly. > > > + ret = acpi_device_write_interrupt_or_gpio(ctx, (struct udevice *)dev, > > + "ready-gpios"); > > + if (ret < 0) > > + return log_msg_ret("irq_gpio", ret); > > I looked a bit closer at the acpi_table.c and would like to emphasize on > lessons learn from BIOS mistakes found in the wild with GPIOs. > > Because GPIO resources are quite badly described in ACPI (it seems MS failed to > deliver GPIO abstraction to ACPI and to Windows API), there are some corner > cases, in order to mitigate which we need to consider the following to avoid > potential glitches and misconfiguration: > > - GpioIo() doesn't have any means of Active Low / High setting, the _DSD must > be provided to mitigate this. > > - GpioIo() doesn't properly communicate the initial state of the output pin, > thus Linux assumes the simple rule: > > Pull Bias Polarity Requested... > > Implicit x AS IS (assumed firmware configured for us) > Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low), > assuming non-active (Polarity = !Pull Bias) > > Down Low as low, assuming active > Down High as high, assuming non-active > Up Low as low, assuming non-active > Up High as high, assuming active > > Hopefully this helps (and maybe can be added to some documentation). > > P.S. Why I2cSerialBus() and not I2cSerialBusV2() ? > Will you address Andy's comments by adding documentation in the next version? Regards, Bin
On Mon, Sep 21, 2020 at 2:51 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sun, Sep 06, 2020 at 03:43:47PM -0600, Simon Glass wrote: > > Generate ACPI information for this device so that Linux can use it > > correctly. > > > + ret = acpi_device_write_interrupt_or_gpio(ctx, (struct udevice *)dev, > > + "ready-gpios"); > > + if (ret < 0) > > + return log_msg_ret("irq_gpio", ret); > > I looked a bit closer at the acpi_table.c and would like to emphasize on > lessons learn from BIOS mistakes found in the wild with GPIOs. > > Because GPIO resources are quite badly described in ACPI (it seems MS failed to > deliver GPIO abstraction to ACPI and to Windows API), there are some corner > cases, in order to mitigate which we need to consider the following to avoid > potential glitches and misconfiguration: > > - GpioIo() doesn't have any means of Active Low / High setting, the _DSD must > be provided to mitigate this. > > - GpioIo() doesn't properly communicate the initial state of the output pin, > thus Linux assumes the simple rule: > > Pull Bias Polarity Requested... > > Implicit x AS IS (assumed firmware configured for us) > Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low), > assuming non-active (Polarity = !Pull Bias) > Down Low as low, assuming active > Down High as high, assuming non-active > Up Low as low, assuming non-active > Up High as high, assuming active I re-read the above piece of the table and found that I mistakenly placed words after 'as ' part. Should be as low, ... as low, ... as high, ... as high, ... So, request follows the bias setting, but polarity, if explicitly present, defines active/non-active state. > Hopefully this helps (and maybe can be added to some documentation). > > P.S. Why I2cSerialBus() and not I2cSerialBusV2() ? > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/tpm/cr50_i2c.c b/drivers/tpm/cr50_i2c.c index 1942c07c605..64831a42232 100644 --- a/drivers/tpm/cr50_i2c.c +++ b/drivers/tpm/cr50_i2c.c @@ -14,11 +14,14 @@ #include <log.h> #include <spl.h> #include <tpm-v2.h> +#include <acpi/acpigen.h> +#include <acpi/acpi_device.h> #include <asm/gpio.h> #include <asm/io.h> #include <asm/arch/iomap.h> #include <asm/arch/pm.h> #include <linux/delay.h> +#include <dm/acpi.h> enum { TIMEOUT_INIT_MS = 30000, /* Very long timeout for TPM init */ @@ -581,6 +584,53 @@ static int cr50_i2c_cleanup(struct udevice *dev) return 0; } +static int cr50_acpi_fill_ssdt(const struct udevice *dev, struct acpi_ctx *ctx) +{ + char scope[ACPI_PATH_MAX]; + char name[ACPI_NAME_MAX]; + const char *hid; + int ret; + + ret = acpi_device_scope(dev, scope, sizeof(scope)); + if (ret) + return log_msg_ret("scope", ret); + ret = acpi_get_name(dev, name); + if (ret) + return log_msg_ret("name", ret); + + hid = dev_read_string(dev, "acpi,hid"); + if (!hid) + return log_msg_ret("hid", ret); + + /* Device */ + acpigen_write_scope(ctx, scope); + acpigen_write_device(ctx, name); + acpigen_write_name_string(ctx, "_HID", hid); + acpigen_write_name_integer(ctx, "_UID", + dev_read_u32_default(dev, "acpi,uid", 0)); + acpigen_write_name_string(ctx, "_DDN", + dev_read_string(dev, "acpi,ddn")); + acpigen_write_sta(ctx, acpi_device_status(dev)); + + /* Resources */ + acpigen_write_name(ctx, "_CRS"); + acpigen_write_resourcetemplate_header(ctx); + ret = acpi_device_write_i2c_dev(ctx, dev); + if (ret < 0) + return log_msg_ret("i2c", ret); + ret = acpi_device_write_interrupt_or_gpio(ctx, (struct udevice *)dev, + "ready-gpios"); + if (ret < 0) + return log_msg_ret("irq_gpio", ret); + + acpigen_write_resourcetemplate_footer(ctx); + + acpigen_pop_len(ctx); /* Device */ + acpigen_pop_len(ctx); /* Scope */ + + return 0; +} + enum { TPM_TIMEOUT_MS = 5, SHORT_TIMEOUT_MS = 750, @@ -653,6 +703,10 @@ static int cr50_i2c_probe(struct udevice *dev) return 0; } +struct acpi_ops cr50_acpi_ops = { + .fill_ssdt = cr50_acpi_fill_ssdt, +}; + static const struct tpm_ops cr50_i2c_ops = { .open = cr50_i2c_open, .get_desc = cr50_i2c_get_desc, @@ -675,5 +729,6 @@ U_BOOT_DRIVER(cr50_i2c) = { .probe = cr50_i2c_probe, .remove = cr50_i2c_cleanup, .priv_auto_alloc_size = sizeof(struct cr50_priv), + ACPI_OPS_PTR(&cr50_acpi_ops) .flags = DM_FLAG_OS_PREPARE, };
Generate ACPI information for this device so that Linux can use it correctly. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) Changes in v1: - Capitalise ACPI_OPS_PTR - Update for acpi_device_write_i2c_dev() return-value change - Use acpi,ddn instead of acpi,desc drivers/tpm/cr50_i2c.c | 55 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)