Jett Rink has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31682
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the DDST ACPI table for the ISH device, thus making the suffix available to the kernel.
BRANCH=none BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 129 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31682/1
diff --git a/src/drivers/intel/ish/Kconfig b/src/drivers/intel/ish/Kconfig new file mode 100644 index 0000000..b4c92ef --- /dev/null +++ b/src/drivers/intel/ish/Kconfig @@ -0,0 +1,6 @@ +config DRIVERS_INTEL_ISH + bool "Support Intel ISH driver that publishes _DSD information" + depends on ARCH_X86 + help + When enabled, chip driver/intel/ish will publish information to the + SSDT _DSD table for the ISH device. \ No newline at end of file diff --git a/src/drivers/intel/ish/Makefile.inc b/src/drivers/intel/ish/Makefile.inc new file mode 100644 index 0000000..cab2b1d8 --- /dev/null +++ b/src/drivers/intel/ish/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_INTEL_ISH) += ish.c diff --git a/src/drivers/intel/ish/chip.h b/src/drivers/intel/ish/chip.h new file mode 100644 index 0000000..426e6bf --- /dev/null +++ b/src/drivers/intel/ish/chip.h @@ -0,0 +1,24 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * 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_device.h> + +/* + * Intel Integrated Sensor Hub (ISH) + */ +struct drivers_intel_ish_config { + /* Firmware variant suffix used by kernel for loading */ + const char *firmware_variant; +}; diff --git a/src/drivers/intel/ish/ish.c b/src/drivers/intel/ish/ish.c new file mode 100644 index 0000000..963eb05 --- /dev/null +++ b/src/drivers/intel/ish/ish.c @@ -0,0 +1,97 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * 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_device.h> +#include <arch/acpigen.h> +#include <console/console.h> +#include <device/pci.h> +#include <device/pci_ids.h> +#include "chip.h" + +static void ish_fill_ssdt_generator(struct device *dev) +{ + struct drivers_intel_ish_config *config = dev->chip_info; + struct device *root = dev->bus->dev; + struct acpi_dp *dsd; + + if (!dev->enabled || !config || !config->firmware_variant) + return; + + acpigen_write_scope(acpi_device_scope(root)); + acpigen_write_device(acpi_device_name(root)); + + dsd = acpi_dp_new_table("_DSD"); + acpi_dp_add_string(dsd, "firmware-variant", config->firmware_variant); + acpi_dp_write(dsd); + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: Set firmware-variant: %s\n", + acpi_device_path(root), config->firmware_variant); +} + +static struct device_operations intel_ish_ops = { + .read_resources = DEVICE_NOOP, + .set_resources = DEVICE_NOOP, + .enable_resources = DEVICE_NOOP, + .acpi_fill_ssdt_generator = ish_fill_ssdt_generator, +}; + +static void intel_ish_enable(struct device *dev) +{ + /* This dev is a generic device that is a child to the ISH PCI device */ + dev->ops = &intel_ish_ops; +} + +static struct pci_driver ish_intel_driver __pci_driver = { + .vendor = PCI_VENDOR_ID_INTEL, + /* .devices and .ops are during chip init if needed */ +}; + +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_ISHB, + 0 +}; + +static void ish_chip_init_update_pci_driver(void *unused) +{ + static struct device_operations pci_ish_device_ops; + + /* + * We know this chip driver is in use, so we need to enable the PCI + * driver to use the default pci ops with the addition of generic scan + * bus. We also make the PCI driver match the ISH PIDs. + * + * If this method is not called (i.e. the Intel ISH chip is not in use), + * then the PCI driver will not match anything on the system. + */ + + /* Use the default pci ops for ISH */ + memcpy(&pci_ish_device_ops, &default_pci_ops_dev, + sizeof(pci_ish_device_ops)); + /* Add generic bus scan */ + pci_ish_device_ops.scan_bus = &scan_generic_bus; + + /* Set new ops and enable PCI driver by setting device IDs */ + ish_intel_driver.ops = &pci_ish_device_ops; + ish_intel_driver.devices = pci_device_ids; +} + +struct chip_operations drivers_intel_ish_ops = { + CHIP_NAME("Intel ISH Chip") + .init = ish_chip_init_update_pci_driver, + .enable_dev = intel_ish_enable, +}; diff --git a/src/include/device/pci_ids.h b/src/include/device/pci_ids.h index 90b02cb..dc5e6a8 100644 --- a/src/include/device/pci_ids.h +++ b/src/include/device/pci_ids.h @@ -2091,6 +2091,7 @@ #define PCI_DEVICE_ID_INTEL_80960_RP 0x1960 #define PCI_DEVICE_ID_INTEL_82437VX 0x7030 #define PCI_DEVICE_ID_INTEL_82439TX 0x7100 +#define PCI_DEVICE_ID_INTEL_ISHB 0x9dfc
/* Intel 82371FB (PIIX) */ #define PCI_DEVICE_ID_INTEL_82371FB_ISA 0x122e
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig File src/drivers/intel/ish/Kconfig:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig@2 PS1, Line 2: bool "Support Intel ISH driver that publishes _DSD information" Do we want this user selectable or just selected automatically in SoC or mainboard Kconfig?
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig File src/drivers/intel/ish/Kconfig:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig@2 PS1, Line 2: bool "Support Intel ISH driver that publishes _DSD information"
Do we want this user selectable or just selected automatically in SoC or mainboard Kconfig?
I had it selectable so you don't need extra drivers.
For sarien, I only include this for the arcada build. See https://review.coreboot.org/c/coreboot/+/31683/1/src/mainboard/google/sarien...
Hello Patrick Rudolph, Duncan Laurie, 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/+/31682
to look at the new patch set (#2).
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the DDST ACPI table for the ISH device, thus making the suffix available to the kernel.
BRANCH=none BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 129 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31682/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@29 PS1, Line 29: if (!dev->enabled || !config || !config->firmware_variant) Shouldn't we check if root->enabled is true as well? Or does the device walking already take that into account? looks like it doesn't such notions:
-------{ ------->-------struct device *dev; ------->-------for (dev = all_devices; dev; dev = dev->next) ------->------->-------if (dev->ops && dev->ops->acpi_fill_ssdt_generator) ------->------->------->-------dev->ops->acpi_fill_ssdt_generator(dev); ------->-------current = (unsigned long) acpigen_get_current(); -------}
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@71 PS1, Line 71: static struct device_operations pci_ish_device_ops; Why aren't you working on the intel_ish_ops directly? The path below NULLs out acpi_fill_ssdt_generator and then at enable_dev time you're switching ops out again. This is really bizarre. The ops needs to be a composition of the default pci device ops with only updating .scan_bus field.
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@90 PS1, Line 90: ish_intel_driver.devices = pci_device_ids; Why is this being initialized again? Just put those in the static initializer.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig File src/drivers/intel/ish/Kconfig:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig@2 PS1, Line 2: bool "Support Intel ISH driver that publishes _DSD information"
I had it selectable so you don't need extra drivers. […]
When there is a quoted string after the Kconfig type this option shows up in the Kconfig gui. That's what I mean by user selectable. Your selection in the link provided will work regardless of a quoted string or not.
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig@3 PS1, Line 3: depends on ARCH_X86 I don't think this is really necessary in practice.
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig File src/drivers/intel/ish/Kconfig:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig@3 PS1, Line 3: depends on ARCH_X86
I don't think this is really necessary in practice.
Should I remove it? I just saw another driver add this. I don't have a preference
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@29 PS1, Line 29: if (!dev->enabled || !config || !config->firmware_variant)
Shouldn't we check if root->enabled is true as well? Or does the device walking already take that in […]
I tested this out, and if you disable the parent device (ISH PCI device), then this code is not executed. We only need to check the current device status.
I will poke around more, but I am guessing that the all_devices linked list is updated to removed disabled devices at some point before SSDT generation
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@71 PS1, Line 71: static struct device_operations pci_ish_device_ops;
Why aren't you working on the intel_ish_ops directly? The path below NULLs out acpi_fill_ssdt_genera […]
there are multiple devices in the file (I tried to clarify with comment on line 55).
These device operations are the ISH PCI device operations, which want to be the default_pci_ops_dev operations with the addition of scan_generic_bus. That is accomplished in this function.
I do not want intel_ish_ops here since those operations are for the generic device that is nested under the ISH PCI device. That generic device is the one that handles the SSDT table creation.
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@90 PS1, Line 90: ish_intel_driver.devices = pci_device_ids;
Why is this being initialized again? Just put those in the static initializer.
If the chip constructed is never used in devicetree.cb (meaning someone just turned on the ISH without adding a chip clause underneath it in the devicetree.cb), then I do not want the ISH PCI driver override to actual match or override any behavior - it goes unused.
Only if you actually use the chip instance do we need to have the pci driver hook that will allow us to inject the scan_generic_bus method
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@71 PS1, Line 71: static struct device_operations pci_ish_device_ops;
there are multiple devices in the file (I tried to clarify with comment on line 55). […]
Ya. As we spoke I think the back and forth makes it more complicated to avoid duplicating default_pci_ops_dev field. There are some things we can do in struct device_operations and the call sites that allow overriding certain fields, but we don't have that in place.
Hello Patrick Rudolph, Duncan Laurie, 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/+/31682
to look at the new patch set (#3).
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the SSDT ACPI table for the ISH device, thus making the suffix available to the kernel.
BRANCH=none BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 119 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31682/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31682/3/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/3/src/drivers/intel/ish/ish.c@81 PS3, Line 81: .vendor = PCI_VENDOR_ID_INTEL, please, no space before tabs
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig File src/drivers/intel/ish/Kconfig:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig@2 PS1, Line 2: bool "Support Intel ISH driver that publishes _DSD information"
When there is a quoted string after the Kconfig type this option shows up in the Kconfig gui. […]
Done
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/Kconfig@3 PS1, Line 3: depends on ARCH_X86
Should I remove it? I just saw another driver add this. […]
Done
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/1/src/drivers/intel/ish/ish.c@71 PS1, Line 71: static struct device_operations pci_ish_device_ops;
Ya. […]
Changes code to be statically complied for everything instead
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31682/3/src/include/device/pci_ids.h File src/include/device/pci_ids.h:
https://review.coreboot.org/#/c/31682/3/src/include/device/pci_ids.h@2094 PS3, Line 2094: #define PCI_DEVICE_ID_INTEL_ISHB 0x9dfc 0x9dfc only for ISH for cannonlake pch, change to PCI_DEVICE_ID_INTEL_CNL_ISHB instead?
Hello Patrick Rudolph, Duncan Laurie, 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/+/31682
to look at the new patch set (#4).
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the SSDT ACPI table for the ISH device, thus making the suffix available to the kernel.
BRANCH=none BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 126 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31682/4
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31682/3/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/3/src/drivers/intel/ish/ish.c@81 PS3, Line 81: .vendor = PCI_VENDOR_ID_INTEL,
please, no space before tabs
Done
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 4: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/chip.h File src/drivers/intel/ish/chip.h:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/chip.h@16 PS4, Line 16: #include <arch/acpi_device.h> This include is not needed here.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@40 PS4, Line 40: acpigen_write_name_dword("_ADR", address); Why are we adding in an _ADR field if it's already in the static asl file?
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@73 PS4, Line 73: .acpi_fill_ssdt_generator = pci_rom_ssdt, This device will never have a rom.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@77 PS4, Line 77: .enable = 0, no need to fill out fields that are 0 value.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@78 PS4, Line 78: .ops_pci = &pci_dev_ops_pci, This will get set during probe.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@33 PS4, Line 33: acpigen_write_scope(acpi_device_scope(root)); : acpigen_write_device(acpi_device_name(root)); This will end up adding ISHB device again to the ACPI tree -- probably resulting in parsing errors in kernel and also associating the _DSD with incorrect device. This should just emit scope here:
acpigen_write_scope(acpi_device_path(root));
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@40 PS4, Line 40: acpigen_write_name_dword("_ADR", address);
Why are we adding in an _ADR field if it's already in the static asl file?
Yes, this should not be required.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@46 PS4, Line 46: acpigen_pop_len(); /* Device */ Not required.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@40 PS4, Line 40: acpigen_write_name_dword("_ADR", address);
Yes, this should not be required.
Actually since we are adding _ADR here, can we get rid of the static .asl file completely?
rushikesh s kadam has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@43 PS4, Line 43: firmware-variant Should this be "firmware-name" as in kernel driver here https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1...
Hello Patrick Rudolph, Duncan Laurie, Lijian Zhao, 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/+/31682
to look at the new patch set (#5).
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the SSDT ACPI table for the ISH device, thus making the suffix available to the kernel.
BRANCH=none BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31682/5
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/chip.h File src/drivers/intel/ish/chip.h:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/chip.h@16 PS4, Line 16: #include <arch/acpi_device.h>
This include is not needed here.
Done
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@33 PS4, Line 33: acpigen_write_scope(acpi_device_scope(root)); : acpigen_write_device(acpi_device_name(root));
This will end up adding ISHB device again to the ACPI tree -- probably resulting in parsing errors i […]
Update to just use scope since the rest should be defined in asl statically already
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@40 PS4, Line 40: acpigen_write_name_dword("_ADR", address);
Actually since we are adding _ADR here, can we get rid of the static . […]
I was thinking that someone might come along later and add a static ISH asl define and then we would have 2 devices again. It seems a little safer to statically define the ISH device in ASL (since the device is statically known - unlike extra i2c devices), and add the _DSD table in SSDT
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@43 PS4, Line 43: firmware-variant
Should this be "firmware-name" as in kernel driver here […]
I think we should use firmware-variant as a suffix and keep the calculated ish5.0-<variant>-fw.bin part of the firmware name.
This will also allow the Intel driver to guard loading the wrong version of ISH code on a HW block
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@46 PS4, Line 46: acpigen_pop_len(); /* Device */
Not required.
Not required at all, or not required because I am only using one acpigen_write_(scope|device)?
I am only using one pop_len now, if I don't need to use that one let me know
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@73 PS4, Line 73: .acpi_fill_ssdt_generator = pci_rom_ssdt,
This device will never have a rom.
Removed
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@77 PS4, Line 77: .enable = 0,
no need to fill out fields that are 0 value.
Removed. I was just keeping it the same as default_pci_ops_dev
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@78 PS4, Line 78: .ops_pci = &pci_dev_ops_pci,
This will get set during probe.
I didn't find that, can you point it out? To me it looks like this device_operations structure is taken whole or not. I didn't see any point were the ops_pci was overriden.
rushikesh s kadam has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@43 PS4, Line 43: firmware-variant
I think we should use firmware-variant as a suffix and keep the calculated ish5.0-<variant>-fw. […]
Hi Jett, I was thinking on similar lines, and that is reflected in my earlier driver implementation. But I received feedback on the Intel internal Linux mailing list against using a custom property (firmware-variant or ish-platform in my case). Likewise using filename as a check against loading bad FW wasn't well received. Let me know what you think.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@43 PS4, Line 43: firmware-variant
Hi Jett, I was thinking on similar lines, and that is reflected in my earlier driver implementation. […]
Rushikesh, what was the alternate proposal? And did the internal mailing list understand that different projects will one utilize different fw and provide different functionality? What was their proposal for disambiguating the correct firmware to load -- as well as binding the proper kernel drive based on implementation?
rushikesh s kadam has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@43 PS4, Line 43: firmware-variant
Rushikesh, what was the alternate proposal? And did the internal mailing list understand that differ […]
Hi Aaron, the proposal to use _DSD() method to pass the firmware-name device property (as is implemented in this patch) was approved. So that's good.
My comment is about - (1) WE want to use an existing kernel device property name firmware-name instead of a custom platform-variant, (2) firmware-name property should return full filename
Sample code https://partnerissuetracker.corp.google.com/issues/123047899#comment1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31682/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31682/5//COMMIT_MSG@14 PS5, Line 14: to the kernel Please add an URL to the corresponding Linux kernel change.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@46 PS4, Line 46: acpigen_pop_len(); /* Device */
Not required at all, or not required because I am only using one acpigen_write_(scope|device)? […]
Not required because you just need scope and not device.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@72 PS4, Line 72: .write_acpi_tables = pci_rom_write_acpi_tables, This won't be required as well.
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@43 PS4, Line 43: firmware-variant
Hi Aaron, the proposal to use _DSD() method to pass the firmware-name device property (as is impleme […]
is firmware-name already an approved _DSD entry that we are reusing? If not don't we have to register a new _DSD entry either way?
I think firmware-variant gives us the right amount of control and still leaving the right amount of control in the kernel driver (deciding ish 5.0 etc).
Can you link to the push back you got for something other than firmware-name (or where it got approved)?
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@72 PS4, Line 72: .write_acpi_tables = pci_rom_write_acpi_tables,
This won't be required as well.
will remove
Hello Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31682
to look at the new patch set (#6).
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the SSDT ACPI table for the ISH device, thus making the suffix available to the kernel.
BRANCH=none BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 110 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31682/6
Hello Patrick Rudolph, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31682
to look at the new patch set (#7).
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the SSDT ACPI table for the ISH device, thus making the suffix available to the kernel (See crrev.com/c/1433482 for kernel change that uses the value)
BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 110 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31682/7
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/#/c/31682/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31682/5//COMMIT_MSG@14 PS5, Line 14: to the kernel
Please add an URL to the corresponding Linux kernel change.
Done
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@43 PS4, Line 43: firmware-variant
is firmware-name already an approved _DSD entry that we are reusing? If not don't we have to registe […]
Based on offline discussion, using firmware-name for now to make forward progress.
https://review.coreboot.org/#/c/31682/4/src/drivers/intel/ish/ish.c@72 PS4, Line 72: .write_acpi_tables = pci_rom_write_acpi_tables,
will remove
Done
https://review.coreboot.org/#/c/31682/3/src/include/device/pci_ids.h File src/include/device/pci_ids.h:
https://review.coreboot.org/#/c/31682/3/src/include/device/pci_ids.h@2094 PS3, Line 2094: #define PCI_DEVICE_ID_INTEL_ISHB 0x9dfc
0x9dfc only for ISH for cannonlake pch, change to PCI_DEVICE_ID_INTEL_CNL_ISHB instead?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
driver/intel/ish: add ish chip driver support
We want to be able to specify the firmware variant suffix in the devicetree.cb configuration for particular firmware builds. This driver allows us to specify the firmware_variant property in the device tree and have it populate a _DST table in the SSDT ACPI table for the ISH device, thus making the suffix available to the kernel (See crrev.com/c/1433482 for kernel change that uses the value)
BUG=b:122722008 TEST=decompile DDST table and verify that new firmware-variant value is present. Also verfied that kernel can access this new field using the shim loader kernel CLs
Change-Id: Id8be986185282521aee574027503eaf8968e1508 Signed-off-by: Jett Rink jettrink@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/31682 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- A src/drivers/intel/ish/Kconfig A src/drivers/intel/ish/Makefile.inc A src/drivers/intel/ish/chip.h A src/drivers/intel/ish/ish.c M src/include/device/pci_ids.h 5 files changed, 110 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/intel/ish/Kconfig b/src/drivers/intel/ish/Kconfig new file mode 100644 index 0000000..635864e --- /dev/null +++ b/src/drivers/intel/ish/Kconfig @@ -0,0 +1,5 @@ +config DRIVERS_INTEL_ISH + bool + help + When enabled, chip driver/intel/ish will publish information to the + SSDT _DSD table for the ISH device. diff --git a/src/drivers/intel/ish/Makefile.inc b/src/drivers/intel/ish/Makefile.inc new file mode 100644 index 0000000..cab2b1d8 --- /dev/null +++ b/src/drivers/intel/ish/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_INTEL_ISH) += ish.c diff --git a/src/drivers/intel/ish/chip.h b/src/drivers/intel/ish/chip.h new file mode 100644 index 0000000..ae3fb35 --- /dev/null +++ b/src/drivers/intel/ish/chip.h @@ -0,0 +1,22 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * 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. + */ + +/* + * Intel Integrated Sensor Hub (ISH) + */ +struct drivers_intel_ish_config { + /* Firmware name used by kernel for loading ISH firmware */ + const char *firmware_name; +}; diff --git a/src/drivers/intel/ish/ish.c b/src/drivers/intel/ish/ish.c new file mode 100644 index 0000000..bc1b6fa --- /dev/null +++ b/src/drivers/intel/ish/ish.c @@ -0,0 +1,81 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * 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_device.h> +#include <arch/acpigen.h> +#include <console/console.h> +#include <device/pci.h> +#include <device/pci_ids.h> +#include "chip.h" + +static void ish_fill_ssdt_generator(struct device *dev) +{ + struct drivers_intel_ish_config *config = dev->chip_info; + struct device *root = dev->bus->dev; + struct acpi_dp *dsd; + + if (!dev->enabled || !config || !config->firmware_name) + return; + + acpigen_write_scope(acpi_device_path(root)); + + dsd = acpi_dp_new_table("_DSD"); + acpi_dp_add_string(dsd, "firmware-name", config->firmware_name); + acpi_dp_write(dsd); + + acpigen_pop_len(); /* Scope */ + + printk(BIOS_INFO, "%s: Set firmware-name: %s\n", + acpi_device_path(root), config->firmware_name); +} + +static struct device_operations intel_ish_ops = { + .read_resources = DEVICE_NOOP, + .set_resources = DEVICE_NOOP, + .enable_resources = DEVICE_NOOP, + .acpi_fill_ssdt_generator = ish_fill_ssdt_generator, +}; + +static void intel_ish_enable(struct device *dev) +{ + /* This dev is a generic device that is a child to the ISH PCI device */ + dev->ops = &intel_ish_ops; +} + +/* Copy of default_pci_ops_dev with scan_bus addition */ +static const struct device_operations pci_ish_device_ops = { + .read_resources = pci_dev_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = pci_dev_enable_resources, + .init = pci_dev_init, + .scan_bus = &scan_generic_bus, /* Non-default */ + .ops_pci = &pci_dev_ops_pci, +}; + +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_CNL_ISHB, + 0 +}; + +static const struct pci_driver ish_intel_driver __pci_driver = { + .ops = &pci_ish_device_ops, + .vendor = PCI_VENDOR_ID_INTEL, + .devices = pci_device_ids, +}; + +struct chip_operations drivers_intel_ish_ops = { + CHIP_NAME("Intel ISH Chip") + .enable_dev = intel_ish_enable, +}; diff --git a/src/include/device/pci_ids.h b/src/include/device/pci_ids.h index 0fd45e3..45d9c14 100644 --- a/src/include/device/pci_ids.h +++ b/src/include/device/pci_ids.h @@ -2091,6 +2091,7 @@ #define PCI_DEVICE_ID_INTEL_80960_RP 0x1960 #define PCI_DEVICE_ID_INTEL_82437VX 0x7030 #define PCI_DEVICE_ID_INTEL_82439TX 0x7100 +#define PCI_DEVICE_ID_INTEL_CNL_ISHB 0x9dfc
/* Intel 82371FB (PIIX) */ #define PCI_DEVICE_ID_INTEL_82371FB_ISA 0x122e
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c@63 PS8, Line 63: scan_generic_bus Do we expect devices behind the ISH? Is it a bridge to something that would be specified in the devicetree?
Sorry if that was already answered during review (I found this mentioned but not actually why it is used).
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c@63 PS8, Line 63: scan_generic_bus
Do we expect devices behind the ISH? Is it a bridge to […]
We expect a statically defined, generic device of chip type drivers/intel/ish to be defined in device tree underneath the ISH PCI device.
(the chip is defined here with a single field firmware_name, which the value is used to publish a firmware-name _DSD value up to the AP)
We need generic scan bus otherwise the ISH PCI device won't enumerate the generic, drivers/intel/ish device defined underneath it in the device tree.
See https://review.coreboot.org/c/coreboot/+/31683/8/src/mainboard/google/sarien... for an example usage.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c@63 PS8, Line 63: scan_generic_bus
We expect a statically defined, generic device of chip type drivers/intel/ish to be defined in devic […]
I see, thank you :)
Background: I'm trying to clean up around scan_static_bus() and scan_generic_bus(). The only difference seems to be that the latter assigns bus numbers. It doesn't seem like we need that here. It's just confusing to have two versions with no clear rule when to use what.
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c@63 PS8, Line 63: scan_generic_bus
I see, thank you :) […]
Ah, I didn't see that other version. We could definitely use the scan_static_bus function instead. Should you are I update the code to use scan_static_bus instead?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31682 )
Change subject: driver/intel/ish: add ish chip driver support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c File src/drivers/intel/ish/ish.c:
https://review.coreboot.org/#/c/31682/8/src/drivers/intel/ish/ish.c@63 PS8, Line 63: scan_generic_bus
Ah, I didn't see that other version. We could definitely use the scan_static_bus function instead. […]
Doesn't matter. Let's just leave this as is for now.