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

(24 comments)

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

Line 62: 	size_t count);
> Comment your exposed functions. Really his entire file needs better comment
Done


Line 67: #if ENV_RAMSTAGE
> I really hate guarding things like this, and I've yet to see a good reason 
Done


Line 70: int soc_add_fixed_mmio_resources(device_t dev);
> Don't use device_t here. use struct device *.
Done


Line 76: 	size_t size;
> What's the significance of this filed? Like functions, these exposed struct
Done


PS5, Line 85: int
> bool?
Done


PS5, Line 94: BAR
> at REG in pci space? Otherwise it's purely a value found at reg?
Done


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;
> we should use index as return
Done


Line 46: __attribute__((weak)) void soc_add_imr_resources(device_t dev, int index)
> got it, you want to unify behavior, i had focused on function caling order.
Done


Line 55: #define  DPR_SIZE_MASK	0xff0
> Yes, I know. It was more about how this doesn't apply to all chipsets. A co
Done


Line 57: int sa_get_pcie_base_addr(device_t dev, unsigned int index, uintptr_t *base,
> But you don't have to.
Done


PS5, Line 84: if(
> space between if and (
Done


PS5, Line 93: len
> i guess this is a miss, we don't need to get the len, purpose for this API 
Done


PS5, Line 115: *len)
> But we know base and length for nearly all the entries in such a table. We 
Done


Line 156: 				  IORESOURCE_ASSIGNED;
> mmio_resource()
Done


Line 166: static void read_map_entry(device_t dev, const struct sa_map_entry *entry,
> But why complicate this further? Why not provide an array of 
Done


Line 306: 			  IORESOURCE_ASSIGNED;
> Why isn't reserved_ram_resource() and ram_resource() being used?
Done


PS5, Line 310: SA_TSEG_REG
> That would be helpful documentation in the header file about the surface ar
Done


PS5, Line 337: (0xa0000 / KiB)
> parens not needed
Done


Line 338: 			(0xc0000 - 0xa0000) / KiB);
> same line
Done


PS5, Line 339: (0xc0000 / KiB),
> parens not needed
Done


PS5, Line 365: const struct sa_mmio_descriptor *sa_imr_resources
> All that was used out of this structure is a index and an optional descript
Done


Line 384: 			resource->base = (base & 0x0fffffff) * KiB;
> Why isn't reserved_ram_resource being used here? Is this so one can spew ou
Done


Line 422: 	int index = 0;
> No reason to set this to 0. It's assigned below.
Done


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

Line 111: 		       (unsigned long)base);
> So we're going to see that these messages spewed out on every boot? What is
i have now guarded against SA_DEBUG, to avoid every boot more debug log. now its more over user debug interest.


-- 
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: Furquan Shaikh <furquan at google.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