Change in ...coreboot[master]: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to gen...
Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32006
Change subject: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate. ......................................................................
fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate.
Fleex has the option of two different touchscreens, WDT or Elan. A new VPD entry will go along with this (key="touchscreen", value="wdt" or "elan"). If the VPD entry exists, only one ACPI node will be generated for that I2C bus. If no entry exists, or no tag is specified in the device tree, then both nodes will be generated and it will be up to the kernel to probe for them.
Change-Id: I0e0a9f96f3d3de9be36b218600268711b80c18b9 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/i2c/generic/chip.h M src/drivers/i2c/generic/generic.c M src/mainboard/google/octopus/variants/fleex/overridetree.cb M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/vpd_ssdt.c A src/vendorcode/google/chromeos/vpd_ssdt.h 6 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/32006/1
diff --git a/src/drivers/i2c/generic/chip.h b/src/drivers/i2c/generic/chip.h index 9e2abb9..7cf04cd 100644 --- a/src/drivers/i2c/generic/chip.h +++ b/src/drivers/i2c/generic/chip.h @@ -75,6 +75,8 @@ /* Generic properties for exporting device-specific data to the OS */ struct acpi_dp property_list[MAX_GENERIC_PROPERTY_LIST]; int property_count; + const char *vpd_string_tag; + const char *vpd_string_value; };
/* diff --git a/src/drivers/i2c/generic/generic.c b/src/drivers/i2c/generic/generic.c index 598f211..75a365f 100644 --- a/src/drivers/i2c/generic/generic.c +++ b/src/drivers/i2c/generic/generic.c @@ -22,6 +22,8 @@ #include <gpio.h> #include <stdint.h> #include <string.h> +#include <vendorcode/google/chromeos/vpd_ssdt.h> + #include "chip.h"
#if CONFIG(HAVE_ACPI_TABLES) @@ -79,6 +81,11 @@ return; }
+ if (config->vpd_string_tag && config->vpd_string_value && + !vpd_should_generate_ssdt(config->vpd_string_tag, + config->vpd_string_value)) + return; + /* Device */ acpigen_write_scope(scope); acpigen_write_device(acpi_device_name(dev)); diff --git a/src/mainboard/google/octopus/variants/fleex/overridetree.cb b/src/mainboard/google/octopus/variants/fleex/overridetree.cb index 19246e0..a368598 100644 --- a/src/mainboard/google/octopus/variants/fleex/overridetree.cb +++ b/src/mainboard/google/octopus/variants/fleex/overridetree.cb @@ -122,6 +122,8 @@ register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPIO_146)" register "enable_delay_ms" = "1" register "has_power_resource" = "1" + register "vpd_string_tag" = ""touchscreen"" + register "vpd_string_value" = ""elan"" device i2c 10 on end end chip drivers/i2c/hid @@ -136,6 +138,8 @@ register "generic.has_power_resource" = "1" register "generic.disable_gpio_export_in_crs" = "1" register "hid_desc_reg_offset" = "0x20" + register "generic.vpd_string_tag" = ""touchscreen"" + register "generic.vpd_string_value" = ""wdt"" device i2c 2c on end end end # - I2C 7 diff --git a/src/vendorcode/google/chromeos/Makefile.inc b/src/vendorcode/google/chromeos/Makefile.inc index 000d056..6776c5f 100644 --- a/src/vendorcode/google/chromeos/Makefile.inc +++ b/src/vendorcode/google/chromeos/Makefile.inc @@ -18,6 +18,7 @@ ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi.c ramstage-$(CONFIG_CHROMEOS_RAMOOPS) += ramoops.c ramstage-y += vpd_mac.c vpd_serialno.c vpd_calibration.c +ramstage-$(CONFIG_HAVE_ACPI_TABLES) += vpd_ssdt.c ramstage-$(CONFIG_CHROMEOS_DISABLE_PLATFORM_HIERARCHY_ON_RESUME) += tpm2.c ramstage-$(CONFIG_HAVE_REGULATORY_DOMAIN) += wrdd.c ramstage-$(CONFIG_USE_SAR) += sar.c diff --git a/src/vendorcode/google/chromeos/vpd_ssdt.c b/src/vendorcode/google/chromeos/vpd_ssdt.c new file mode 100644 index 0000000..7ed0199 --- /dev/null +++ b/src/vendorcode/google/chromeos/vpd_ssdt.c @@ -0,0 +1,53 @@ +/* + * 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 <boot/coreboot_tables.h> +#include <drivers/vpd/vpd.h> + +#include <string.h> + +#include "vpd_ssdt.h" + +/* + * For multi-sourced components, we may decide to store the presence + * of one or the other in the VPD area. If that's the case, we can + * decide to only populate the ACPI SSDT table with the one that's + * actually in the device in question. In that case, we'll search + * the VPD tables to find a matching entry; if there is an entry + * with the appropriate tag, return true if the value matches, false + * otherwise. If there is no entry, then return true as well, meaning + * we don't know which component is being used, so the entry should be + * available in the SSDT and the kernel can probe for both. + */ +int vpd_should_generate_ssdt(const char *key, const char *value) +{ + const void *vpd_entry; + int size; + int keys_cmp; + + vpd_entry = vpd_find(key, &size, VPD_RO); + + /* If no entry was found, then generate an SSDT node */ + if (!vpd_entry) + return 1; + + /* If an entry was found, then compare values */ + keys_cmp = strncmp((char *)vpd_entry, value, size); + + if (keys_cmp == 0) + return 1; + else + return 0; +} diff --git a/src/vendorcode/google/chromeos/vpd_ssdt.h b/src/vendorcode/google/chromeos/vpd_ssdt.h new file mode 100644 index 0000000..37e887e --- /dev/null +++ b/src/vendorcode/google/chromeos/vpd_ssdt.h @@ -0,0 +1,21 @@ +/* + * 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. + */ + +#ifndef VPD_ACPI_H +#define VPD_ACPI_H + +int vpd_should_generate_ssdt(const char *key, const char *value); + +#endif /* VPD_ACPI_H */
Hello Aaron Durbin, Karthikeyan Ramasubramanian, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32006
to look at the new patch set (#2).
Change subject: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate. ......................................................................
fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate.
Fleex has the option of two different touchscreens, WDT or Elan. A new VPD entry will go along with this (key="touchscreen", value="wdt" or "elan"). If the VPD entry exists, only one ACPI node will be generated for that I2C bus. If no entry exists, or no tag is specified in the device tree, then both nodes will be generated and it will be up to the kernel to probe for them.
BUG=b:121309055 BRANCH=none TEST=builds
Change-Id: I0e0a9f96f3d3de9be36b218600268711b80c18b9 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/i2c/generic/chip.h M src/drivers/i2c/generic/generic.c M src/mainboard/google/octopus/variants/fleex/overridetree.cb M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/vpd_ssdt.c A src/vendorcode/google/chromeos/vpd_ssdt.h 6 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/32006/2
Hello Aaron Durbin, Karthikeyan Ramasubramanian, 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/+/32006
to look at the new patch set (#3).
Change subject: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate. ......................................................................
fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate.
Fleex has the option of two different touchscreens, WDT or Elan. A new VPD entry will go along with this (key="touchscreen", value="wdt" or "elan"). If the VPD entry exists, only one ACPI node will be generated for that I2C bus. If no entry exists, or no tag is specified in the device tree, then both nodes will be generated and it will be up to the kernel to probe for them.
BUG=b:121309055 BRANCH=none TEST=compiles
Change-Id: I0e0a9f96f3d3de9be36b218600268711b80c18b9 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/i2c/generic/chip.h M src/drivers/i2c/generic/generic.c M src/mainboard/google/octopus/variants/fleex/overridetree.cb M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/vpd_ssdt.c A src/vendorcode/google/chromeos/vpd_ssdt.h 6 files changed, 88 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/32006/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32006 )
Change subject: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate. ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... File src/mainboard/google/octopus/variants/fleex/overridetree.cb:
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... PS3, Line 125: register "vpd_string_tag" = ""touchscreen"" I don't think we should have these in the chip.h.
And, I think there's an issue with the namespace of the keys. A device could have multiple screens. So how do we know what 'touchscreen' ?
The key should be encode i2c controller path with the value of the hid below.
This should be 1:1 in what you'd see in sysfs on a booted system.
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... PS3, Line 126: register "vpd_string_value" = ""elan"" Why make this 'elan' and not just the hid? i.e. ELAN0001
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... File src/vendorcode/google/chromeos/vpd_ssdt.h:
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... PS3, Line 19: int vpd_should_generate_ssdt(const char *key, const char *value); What are the return values?
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... File src/vendorcode/google/chromeos/vpd_ssdt.c:
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... PS3, Line 4: Inc LLC
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32006 )
Change subject: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate. ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... File src/mainboard/google/octopus/variants/fleex/overridetree.cb:
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... PS3, Line 125: register "vpd_string_tag" = ""touchscreen""
I don't think we should have these in the chip.h. […]
What's a better location for this?
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... PS3, Line 126: register "vpd_string_value" = ""elan""
Why make this 'elan' and not just the hid? i.e. […]
Ack
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... File src/vendorcode/google/chromeos/vpd_ssdt.h:
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... PS3, Line 19: int vpd_should_generate_ssdt(const char *key, const char *value);
What are the return values?
Ack
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... File src/vendorcode/google/chromeos/vpd_ssdt.c:
https://review.coreboot.org/#/c/32006/3/src/vendorcode/google/chromeos/vpd_s... PS3, Line 4: Inc
LLC
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32006 )
Change subject: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate. ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... File src/mainboard/google/octopus/variants/fleex/overridetree.cb:
https://review.coreboot.org/#/c/32006/3/src/mainboard/google/octopus/variant... PS3, Line 125: register "vpd_string_tag" = ""touchscreen""
What's a better location for this?
Well, I don't think we need to add these fields to the struct. We can do this in the code only without being prescriptive in the chip section.
There's a couple of things we need at a high level:
Indicate path of bus/device (pci 17.3) in this case for looking at all children and check for vpd entries that match hid and bus/device path. If a vpd entry is there look through all the children and look for a hid match. If one exists all other devices are disabled but that one. If no matches found keep everything enabled as it was in devicetree.
This gets complicated by a couple of things: 1. hierarchy of objects needs to be checked by way of matching chip_ops pointer value against known symbols (drivers_i2c_generic_ops, drivers_i2c_hid_ops). 2. DEVTREE_EARLY currently guards chip_ops field in struct device. This means this work can only be done in ramstage.
I was discussing some of this with Furquan yesterday. #2 above is a concern if one was going to sequence things from earlier stages -- there's no chip ops to disambiguate chip_info pointer.
We should brainstorm some ideas to cover these. Or just do the address probing at run time for the i2c device, but we'll still likely need a lot of the same constructs for power sequencing, etc. The only difference is the vpd lookup vs the probe.
Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32006 )
Change subject: fleex: Add VPD entry for deciding which touchscreen ACPI nodes to generate. ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Tim Wawrzynczak (Code Review)