Change in coreboot[master]: soc/intel/common/block: Add Intel common systemagent support

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/19668 )

Change subject: soc/intel/common/block: Add Intel common systemagent support
......................................................................

Patch Set 5:

(8 comments)

https://review.coreboot.org/#/c/19668/5/src/soc/intel/common/block/systemage...
File src/soc/intel/common/block/systemagent/systemagent.c:

Line 43: 	return 0;
> This should not return 0 since index is the resource index passed in.

we should use index as return

Line 46: __attribute__((weak)) void soc_add_imr_resources(device_t dev, int index)
> It seems odd that this and soc_add_dram_resources() not symmetric in that t

got it, you want to unify behavior, i had focused on function caling order.

Line 55: #define  DPR_SIZE_MASK	0xff0
> These are only for what chipsets?

Mostly core platform

Line 57: int sa_get_pcie_base_addr(device_t dev, unsigned int index, uintptr_t *base,
> Why is this exposed globally?

i guess we are calling this from ramstage https://review.coreboot.org/#/c/19796/3/src/soc/intel/skylake/systemagent.c

PS5, Line 93: len
> len isn't set?

i guess this is a miss, we don't need to get the len, purpose for this API to get the base address

PS5, Line 115: *len)
> again, no len being set. You made this whole API so PCIE mmconfig area leng

i guess we need to get the base address, len is needed only for PCIEX and in order to make this API generic, we have added length field, we will use Kconfig to achieve that and remove this from here

Line 166: static void read_map_entry(device_t dev, const struct sa_map_entry *entry,
> This map_entry concept and sa_mmio_descriptor have become way too complicat

this API is static hence user side shouldn't bother abut how map entry happen. We havn't done anything specific to make this whole stuff complicated, just taken out soc delta specific stuff and provides same input from soc. 
Do you think https://review.coreboot.org/#/c/19796/3/src/soc/intel/skylake/systemagent.c
this soc part is complicated? i had only opinion in mind that when we port new SOC, we should see BWG system map file and know what all range need to program.

PS5, Line 310: SA_TSEG_REG
> So you're reliant on all these enums to be provided by the SoCs?

yes