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

[Shuo Liu (Code Review)]

Attention is currently required from: 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 7:

(7 comments)

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

https://review.coreboot.org/c/coreboot/+/83318/comment/5d42ca0e_97c2b477?usp... :
PS3, Line 78: 		return;
> Yes because the remaining logics are all depends on the value of CAPID_A register.

Done

https://review.coreboot.org/c/coreboot/+/83318/comment/fa7dfcd0_cd140e1f?usp... :
PS3, Line 133: 		.is_limit = CONFIG(TOUUD_LIMIT),
> I think there is no hints, it's specified in EDS. […]

Done

https://review.coreboot.org/c/coreboot/+/83318/comment/bb2742c1_4b4016cb?usp... :
PS3, Line 176: 		value = ALIGN_DOWN(value + entry->align, entry->align);
> The lower bits of the registers are read as 0s but should be treated as 1s, thus aligning up works.

Done

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

https://review.coreboot.org/c/coreboot/+/83318/comment/fad1118a_33561ef6?usp... :
PS7, Line 308: 		return;
maybe no need HAVE_MULTIPLE_DOMAINS and the below logic will work.
if (!is_domain0(dev->upstream)) return.

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

https://review.coreboot.org/c/coreboot/+/83318/comment/6d5d15ac_390b3438?usp... :
PS3, Line 73:  *   IS_LIMIT = If registers/offset indicates address limit or address limit plus 1.
> IS_LIMIT indicates whether the lower bits should be treated as 1s or 0s, although they are read as 0 […]

Not sure if below pattern fits for all limit cases or not.

value | (align - 1)

If yes, we can leave 'IS_LIMIT/is_limit'.
Otherwise, maybe a renaming will work, e.g. filling_low_bits

P.S. limit will not usually indicate address limit + 1 so I think we could be safe to remove that part in the comment block.

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

https://review.coreboot.org/c/coreboot/+/83318/comment/cb2f33b0_4495d68d?usp... :
PS3, Line 133: 	uint32_t tolud = pci_read_config32(SA_DEV_ROOT, TOLUD);
> If calling sa_read_map_entry(), we should pass a device to it, but this function may be called befor […]

Done

https://review.coreboot.org/c/coreboot/+/83318/comment/4eac69ef_523ba9c5?usp... :
PS3, Line 161: 		return ALIGN_DOWN((pci_read_config32(SA_DEV_ROOT, TSEG + 4) +
> Yes because the lower bits are read as 0s but should be treated as 1s.

Can the process is simplified & clarified as:

ALIGN_DOWN(tolud, CONFIG_TOLUD_ALIGNMENT) | (CONFIG_TOLUD_ALIGNMENT - 1)