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.