John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 11:
(11 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@298 PS10, Line 298: and in 64-bit space
It's a 64-bit register and the mask is even narrower, so it's always in 64-bit space.
done.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@315 PS10, Line 315: in 64-bit space.
see above
done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@329 PS10, Line 329: p2sb_dev
This doesn't seem to be used?
Agreed as p2sb_dev was intended to be referred while trying to read ibdf and hbdf. Removed p2sb_dev here.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@333 PS10, Line 333: and in 64-bit space
see above
done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; :
Why not just read the registers instead? That would save us from any […]
p2sb access is blocked after POSTBOOT_SAI, directly dreading of ibdf and hbdf would get invalid values 0xffff. ibdf=pci_read_config16(p2sb_dev, PCH_P2SB_IBDF) hbdf=pci_read_config16(p2sb_dev, PCH_P2SB_HBDF) so we sync with fsp and program ibdf & hbdf here.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@362 PS10, Line 362: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED)
Why this check? are there dependencies between the vtd units?
If VtdDisable is false, sa_set_mch_bar(soc_vtd_resources, ...) would program the base and set the enable bit. Apart from iGFX, I guess the additional devices enable bit check would help NOT generating dmar table if VtdDisable is set true.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 43: 0xfffffff000ull
The mask seems one bit too long. I guess it should be […]
done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 55: { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" },
I guess it won't hurt but this resource doesn't seem to exist on CFL.
It is updated to differentiate platforms.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 59: #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6
Where do these numbers come from? When are the respective registers set?
Those numbers are referred from platforms(cml/cnl/whl/cfl) fsp setting.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
Why have a `VtdDisable` option at all? Nobody seems to use it and what […]
Platform could refer to this VtdDisalble option through devicetree to disable vtd setting and vmx enable/lock configuration.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
The BWG mentions another register to be programmed first in the IPU's […]
I referred to the bios specification which states the IPU device (B0:D5:F0). It would be assumed that fsp configure properly for platforms like Cannonlake. 1. Verify if IPU device is enabled by reading D0F0RE8h[31] and proceed to 2 if this is cleared. 2. If it is found that IPU is disabled or BIOS desires to disable it, then system bios must clear D0.F0.R54h[10]. 3. System bios must set B0.D0.F5R2Ch bits [31:16] and [15:0] to a value of 0x0000. This operation locks this register. 4. System bios must configure B0.D0.F5.R3Ch bits[7:0] to a suitable value for IPU interrupt line routine.