[coreboot-gerrit] [M] Change in coreboot[main]: soc/intel/common/systemagent: Improve systemagent

[Shuo Liu (Code Review)]

Attention is currently required from: Felix Singer, Jérémy Compostella, [e-mail].

Shuo Liu has posted comments on this change by [e-mail]. ( https://review.coreboot.org/c/coreboot/+/83318?usp=email )

Change subject: soc/intel/common/systemagent: Improve systemagent
......................................................................

Patch Set 10:

(5 comments)

File src/soc/intel/common/block/systemagent/systemagent.c:

https://review.coreboot.org/c/coreboot/+/83318/comment/92bc4f5b_5a9eec6c?usp... :
PS10, Line 146: 	value = ALIGN_DOWN(value, 1 * MiB);
If a register is with low 20+ bits reserved (cannot be written into), the align down to 1MB operation is actually not effective. If a register is with low 20- bits reserved, the align down to 1MB operation is actually changing the value.

At the moment, when we are not clear on the direct impact of the aligning down on specific SoCs, we cannot remove it. But we can add an assert here as reminder in logs.

https://review.coreboot.org/c/coreboot/+/83318/comment/c9a5c4a0_104065c3?usp... :
PS10, Line 78: 		return;
Can you move ln77 to ln84?

https://review.coreboot.org/c/coreboot/+/83318/comment/b7acd2db_bb2c7c26?usp... :
PS10, Line 135: 		.align = CONFIG_TOUUD_ALIGNMENT,
No need to add align here

https://review.coreboot.org/c/coreboot/+/83318/comment/26c0ce6a_912bd8cb?usp... :
PS10, Line 178: 		value = ALIGN_DOWN(value, entry->align);
if (entry->is_limit) {
    probing granularity by writing 1s and read back.
    value = (value | (1 < granularity - 1)) + 1;

} else {
    value = ALIGN_DOWN(value, 1 * MiB); -> keep as is
}

And the +1 operation is quite confusing, it had better to be moved to where the value is used.

File src/soc/intel/common/block/systemagent/systemagent_def.h:

https://review.coreboot.org/c/coreboot/+/83318/comment/a11866c4_69a8538a?usp... :
PS3, Line 73:  *   IS_LIMIT = If registers/offset indicates address limit or address limit plus 1.
> That doesn't make sense since all values here should be aligned. […]

Rename is_limit to fix_up_limit?