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