Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 23: DPTF_GENERIC_PARTICIPANT_TYPE_TSR = 0x3,
What about CPU(0x0) and Fan(0x4)?
The CPU and Fan are not exposed as 'Generic' devices in this implementation, they have their own Device types (PTYP). I don't know why the devices are exposed like that.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 24: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif
Isn't this the PCI device address for the CPU thermal device?
Yes.
Can we instead derive this from the struct device *dev?
Well, PCI_DEV(SA_DEV_DPTF) returns 0x20000, so I'm not sure how this ADDR is related to the PCI address, unless it's just intended to be PCI_DEV(...) << 1 ? but I don't get why.
Also, for Atom SoCs, this address is supposed to be 0x1 (see APL) 😞
Are the thermal devices being added under this PCI device?
Nope, they're under _SB.DPTF
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 68: acpigen_write_name_integer("_ADR", CONFIG_DPTF_CPU_ADDR);
acpigen_write_ADR()?
Ack
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 69: is_participant_used(config, DPTF_CPU)
Is it a valid scenario to have DPTF_CPU as unused?
I think you could. If you had the fans all configured as only cooling down TSRs and the passive controls only on TCHG & TSRs. Whether it's a good idea or not is maybe a different question 😊
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 92: acpigen_write_device("TCHG"); : acpigen_write_name("_HID"); : acpigen_emit_eisaid("INT3403"); : acpigen_write_name_integer("_UID", 0); : acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER); : acpigen_write_name_string("_STR", DEFAULT_CHARGER_NAME); : acpigen_write_STA(is_participant_used(config, DPTF_CHARGER) ? : ACPI_STATUS_DEVICE_ALL_ON : : ACPI_STATUS_DEVICE_ALL_OFF);
Should this sequence be converted to a helper function -- dptf_write_generic_participant? It can be […]
Good call.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 104: 1
I believe this is starting from 1 because TCHG above uses UID 0? It might be a good idea to ensure t […]
Sure, I will try to make that more explicit. your suggestion of dptf_write_generic_participant above would make this simpler.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 112: acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_TSR);
Why no "_STR" object?
It's added in a later patchset, with a devicetree option.