Attention is currently required from: Annie Chen, Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Held, Jincheng Li, Lean Sheng Tan, Nill Ge, Patrick Rudolph, Paul Menzel, Shuo Liu, TangYiwei, Tim Chu.
8 comments:
Patchset:
Hi Shuo, thanks for the update. I hope you enjoyed your holidays.
File src/soc/intel/xeon_sp/include/soc/chip_common.h:
Patch Set #17, Line 12: void soc_create_ioat_domains(struct bus * const bus, const STACK_RES * const sr);
No space after the asterisk, please. And the added `const` is not part of the
type (so it's meaningless to the caller and doesn't add information here).
(Where a const is placed is a subtle difference in syntax but a big difference
semantically. When it's about data pointed to, like in `const STACK_RES *`, it
becomes part of the type. When it's placed directly before the parameter name,
it applies to the variable in the function's body. The latter is meaningless
in a forward declaration and the compiler actually must ignore it.)
File src/soc/intel/xeon_sp/spr/ioat.c:
Patch Set #14, Line 75: if (sr->BusLimit - sr->BusBase + 1 < HQM1_BUS_OFFSET + HQM_RESERVED_BUS) {
I believe we still should check
```
if (sr->BusLimit - sr->BusBase + 1 < HQM_BUS_OFFSET + 1 + HQM_RESERVED_BUS)
```
for the non-optional parts.
File src/soc/intel/xeon_sp/spr/ioat.c:
Patch Set #17, Line 50: bus->max_subordinate = bus_limit;
each domain will own 2 busses instead of 1.
Ack.
Thanks, I missed that the ACPI code does an implicit `1 + *_RESERVED_BUS`.
Patch Set #17, Line 92: assert(bus_limit <= sr->BusLimit)
We are dealing with input from another program, hence using assert()
seems wrong (note that the default setting for assertions is weak in coreboot,
so coreboot would continue with invalid hardware configuration).
This is HQM.
Patch Set #17, Line 109: create_ioat_domain(bus, domain_base, bus_base, bus_limit, 0, -1, mem64_base, mem64_limit);
Make CPM1 and HQM1 optional on 3 accelerator SKUs where smaller bus range is provided. P.S. […]
Acknowledged
File src/soc/intel/xeon_sp/spr/soc_acpi.c:
to review
Doesn't `stack_enabled` cover this? Looking at the code below it seems to me
that it was explicitly written such that we always have the device nodes in
the DSDT, even if they are not active.
I would prefer to discuss this in a separate patch.
To view, visit change 78327. To unsubscribe, or for help writing mail filters, visit settings.