[coreboot-gerrit] 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/systemagent/systemagent.c
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


-- 
To view, visit https://review.coreboot.org/19668
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I969ff187e3d4199864cb2e9c9a13f4d04158e27c
Gerrit-PatchSet: 5
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: V Sowmya <v.sowmya at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma at intel.com>
Gerrit-HasComments: Yes