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

[Name of user not set (Code Review)]

Attention is currently required from: Jérémy Compostella, Shuo Liu.

[e-mail] 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:

(6 comments)

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

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

Yes because the remaining logics are all depends on the value of CAPID_A register.

https://review.coreboot.org/c/coreboot/+/83318/comment/3481692d_43a4f116?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  […]

I think there is no hints, it's specified in EDS. On most client platforms, the lower bits should be treated as 0s, so we can set the default Kconfig value to "n" and overwrite it to "y" on specific platform.

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

The lower bits of the registers are read as 0s but should be treated as 1s, thus aligning up works.

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

https://review.coreboot.org/c/coreboot/+/83318/comment/f93d0b36_08faea0b?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 o […]

IS_LIMIT indicates whether the lower bits should be treated as 1s or 0s, although they are read as 0s. I think I should give more comments here.

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

https://review.coreboot.org/c/coreboot/+/83318/comment/a0be6529_b10dada5?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 her […]

If calling sa_read_map_entry(), we should pass a device to it, but this function may be called before device tree is available (e.g., in romstage).

https://review.coreboot.org/c/coreboot/+/83318/comment/75cb19e6_a20001de?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_r […]

Yes because the lower bits are read as 0s but should be treated as 1s.