[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 3:

(8 comments)

File src/soc/intel/common/block/systemagent/Kconfig:

https://review.coreboot.org/c/coreboot/+/83318/comment/c887d602_edc7d2b2?usp... :
PS3, Line 68: 	  treated as 1s.
Seem not quite clear, do you have example for this?

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

https://review.coreboot.org/c/coreboot/+/83318/comment/f932ed0e_1e664562?usp... :
PS3, Line 78: 		return;
If no HAVE_CAPID_A_REGISTER, all remaining logics will not run?

https://review.coreboot.org/c/coreboot/+/83318/comment/51566f97_67678eba?usp... :
PS3, Line 133: 		.is_limit = CONFIG(TOUUD_LIMIT),
Is there any HW hints could be used to decide is_limit? or we have to use a SoC specific Kconfig to specify?

https://review.coreboot.org/c/coreboot/+/83318/comment/cbcdb77f_48d5afc2?usp... :
PS3, Line 176: 		value = ALIGN_DOWN(value + entry->align, entry->align);
this is to align up?

https://review.coreboot.org/c/coreboot/+/83318/comment/8866af84_78efdec4?usp... :
PS3, Line 307: 	if (CONFIG(HAVE_MULTIPLE_DOMAINS) && dev->upstream->secondary != 0)
I guess even no HAVE_MULTIPLE_DOMAINS, if (dev->upstream->secondary != 0) also work. But this is for safe.

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

https://review.coreboot.org/c/coreboot/+/83318/comment/6cbb6b05_beb7f3b4?usp... :
PS3, Line 73:  *   IS_LIMIT = If registers/offset indicates address limit or address limit plus 1.
Not sure if the handling of IS_LIMIT is a common case? Or explicitly explain how will be processed on values with IS_LIMIT here (e.g. to align up)?

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

https://review.coreboot.org/c/coreboot/+/83318/comment/69af878b_0daa62b8?usp... :
PS3, Line 133: 	uint32_t tolud = pci_read_config32(SA_DEV_ROOT, TOLUD);
Is there any possibility to load from sa_read_map_entry instead of re-read from PCI config space here?

https://review.coreboot.org/c/coreboot/+/83318/comment/0ddea97d_2f897e16?usp... :
PS3, Line 161: 		return ALIGN_DOWN((pci_read_config32(SA_DEV_ROOT, TSEG + 4) +
It looks like TSEG + 4 will return a CONFIG_TSEG_ALIGNMENT aligned limit value, but we need to pci_read_config32(SA_DEV_ROOT, TSEG + 4) | (CONFIG_TSEG_ALIGNMENT - 1) to make it fully active?