[coreboot-gerrit] [L] Change in coreboot[main]: soc/intel/xeon_sp: Redesign resource allocation

[Nico Huber (Code Review)]

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.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78327?usp=email )

Change subject: soc/intel/xeon_sp: Redesign resource allocation
......................................................................

Patch Set 17:

(8 comments)

Patchset:

PS17: 
Hi Shuo, thanks for the update. I hope you enjoyed your holidays.

File src/soc/intel/xeon_sp/include/soc/chip_common.h:

https://review.coreboot.org/c/coreboot/+/78327/comment/e9500103_d638c109 :
PS17, 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:

https://review.coreboot.org/c/coreboot/+/78327/comment/a69c4503_9c093b1d :
PS14, 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:

https://review.coreboot.org/c/coreboot/+/78327/comment/49743a05_09554929 :
PS17, 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`.

https://review.coreboot.org/c/coreboot/+/78327/comment/8b97c53a_096fa225 :
PS17, 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).

https://review.coreboot.org/c/coreboot/+/78327/comment/d3aaec21_4b148d8a :
PS17, Line 95: CPM1
This is HQM.

https://review.coreboot.org/c/coreboot/+/78327/comment/456300cb_8c127a26 :
PS17, 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:

https://review.coreboot.org/c/coreboot/+/78327/comment/b9a88af2_7a389c7e :
PS17, Line 254:
> 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.